Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply _internal.utils.appdirs changes to _vendor.appdirs #7501

Merged
merged 6 commits into from
Dec 24, 2019

Conversation

uranusjr
Copy link
Member

The _internal copy is kept for now as a compatibility layer, so the changes are localised around the _vendor changes.

Progress #6040.

Next steps:

  • Change uses in the code base to directly invoke the _vendor version (there are few).
  • Change tests to call the _vendor version. Do we need to? I think it’s a good idea because we’d want to detect incompatible upstream changes (like the appauthor thing), but it’s not strictly required since manual check from maintainers is needed anyway.

@uranusjr
Copy link
Member Author

Err, sorry, I intended to post this as a draft but forgot. Now I don’t know how to change the setting :(

@uranusjr uranusjr force-pushed the appdirs-patch branch 2 times, most recently from c6c83e3 to 8e06859 Compare December 19, 2019 09:03
* Convert Windows app data directory values to bytes on Python 2, so the
  output type is consistent across platforms (pypa#4000)
* Also look in ~/.config for user config on macOS (pypa#4100)
* Remove pywin32 dependency, only use ctypes and winreg for directory
  lookup on Windows (pypa#2467)
* Always use os.path.join() instead of os.sep.join() so cross-platform
  tests work as expected (pypa#3275)
@pradyunsg
Copy link
Member

Err, sorry, I intended to post this as a draft but forgot. Now I don’t know how to change the setting :(

No worries -- you can't change a "normal" PR into a draft one.


Next steps:

Let's do separate PRs for them?

Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this! That issue has been around for some time.

I compared the implementations pretty closely and spotted a few differences, the rest looks OK to me.

src/pip/_internal/utils/appdirs.py Outdated Show resolved Hide resolved
src/pip/_internal/utils/appdirs.py Show resolved Hide resolved
src/pip/_vendor/appdirs.py Show resolved Hide resolved
src/pip/_internal/utils/appdirs.py Show resolved Hide resolved
src/pip/_internal/utils/appdirs.py Outdated Show resolved Hide resolved
@pradyunsg pradyunsg added project: vendored dependency Related to a vendored dependency type: maintenance Related to Development and Maintenance Processes type: refactor Refactoring code labels Dec 21, 2019
@uranusjr
Copy link
Member Author

I think this is ready for the next round of reviews 📨

@uranusjr uranusjr requested a review from chrahunt December 23, 2019 07:23
Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@chrahunt chrahunt merged commit 0292938 into pypa:master Dec 24, 2019
@chrahunt
Copy link
Member

Thanks for tackling this @uranusjr!

@pradyunsg pradyunsg added this to the 20.0 milestone Dec 31, 2019
@uranusjr uranusjr deleted the appdirs-patch branch January 1, 2020 14:06
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jan 31, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation project: vendored dependency Related to a vendored dependency type: maintenance Related to Development and Maintenance Processes type: refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants