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.

Sign up for GitHub

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

Jump to bottom

Warn if a path in PATH starts with tilde during install #7345

Merged
merged 3 commits into from
Nov 15, 2019
Merged

Warn if a path in PATH starts with tilde during install #7345

merged 3 commits into from
Nov 15, 2019

Conversation

mdebi
Copy link
Contributor

@mdebi mdebi commented Nov 12, 2019 •
edited
Loading

Warn if a path in PATH starts with tilde during install

The changes here are as documented at comment for issue #6414

  1. Update pip._internal.wheel.message_about_scripts_not_on_PATH here to check and trace a note as mentioned above if any path in PATH starts with a ~.
  2. Add a test to tests.unit.test_wheel.TestMessageAboutScriptsNotOnPATH here to cover this case.

@mdebi mdebi changed the title Warn if a path in PATH starts with tilde during install #6414 Warn if a path in PATH starts with tilde during install Nov 12, 2019
xavfernandez
xavfernandez reviewed Nov 13, 2019
View reviewed changes
Copy link
Member

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

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

It feels like there might be too many(!) test cases for this :-|

src/pip/_internal/wheel.py Outdated Show resolved Hide resolved
@mdebi
Copy link
Contributor Author

mdebi commented Nov 13, 2019

@xavfernandez, I reduced the number of test cases. Please take a look and let me know if any further change is needed. Thank you for your time.

xavfernandez
xavfernandez approved these changes Nov 13, 2019
View reviewed changes
Copy link
Member

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @mdebi.
I felt like the number of tests was too much but would love to have a second opinion of other maintainers
(10 tests were removed in eded1ca)

@xavfernandez xavfernandez merged commit f6aaba9 into pypa:master Nov 15, 2019
@xavfernandez
Copy link
Member

Thansk @mdebi 👍

@chrahunt chrahunt mentioned this pull request Nov 16, 2019
Closed
@mdebi mdebi deleted the issue_6414 branch November 19, 2019 08:19
@mdebi
Copy link
Contributor Author

mdebi commented Nov 19, 2019

Thank you very much @xavfernandez , @chrahunt for your guidance and help :)

@pradyunsg pradyunsg added the type: enhancement Improvements to functionality label Nov 19, 2019
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Dec 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xavfernandez xavfernandez xavfernandez approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation type: enhancement Improvements to functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mdebi @xavfernandez @pradyunsg