-
Notifications
You must be signed in to change notification settings - Fork 3k
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
utils: fix has_leading_dir
corner case
#5794
Conversation
70c155f
to
5bec881
Compare
While we don't really bump into this code else, it'd probably make sense to do this. |
src/pip/_internal/utils/misc.py
Outdated
@@ -236,6 +236,11 @@ def split_leading_dir(path): | |||
def has_leading_dir(paths): | |||
"""Returns true if all the paths have the same leading path name | |||
(i.e., everything is in one subdirectory in an archive)""" | |||
if not paths: | |||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the docstring for this function is correct, it seems this should be True. The reason is that, in logic, this is said to be “vacuously true”: if something is true if it is true “for all” such things, and there are no things, then it is true for all of them (because there are none it needs to be true for). If this return value needs to be False for behavior reasons, the docstring should be clarified to reflect the additional conditions that need to be met.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, maybe it should read (assuming this is correct): “return True if there is at least one directory and all paths are properly inside it.”
5bec881
to
84935c6
Compare
Amended. |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
84935c6
to
42988fc
Compare
Handle archives with a single file (something that is used by setuptools testsuite).
42988fc
to
5aa9fa0
Compare
Rebased. |
(''' | ||
pkg-1.0/ | ||
pkg-1.0/ | ||
''', True), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something doesn't seem right about the function implementation since, for example, ["pkg-1.0/", "pkg-1.0/"]
returns True but ["pkg-1.0/"]
returns False (see further up for a test case for the latter).
cc @waveform80 |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Handle archives with a single file (something that is used by setuptools' testsuite).