-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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
bpo-18283: Add support for bytes to shutil.which #11818
Conversation
# Check that a given file can be accessed with the correct mode. | ||
# Additionally check that `file` is not a directory, as on Windows | ||
# directories pass the os.access check. | ||
def _access_check(fn, mode): |
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.
Just curious any reason over moving this outside which
?
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.
As an author of the original patch, I can explain: it's inefficient to define a temporary function for each which() call. Moreover, there is no technical reason to use a nested function.
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.
LGTM... well, I wrote the initial patch, so I'm not fair :-)
Just a few suggestions.
Lib/shutil.py
Outdated
|
||
# PATHEXT is necessary to check on Windows. | ||
pathext = os.environ.get("PATHEXT", "").split(os.pathsep) | ||
if isinstance(cmd, bytes): | ||
pathext = list(map(os.fsencode, pathext)) |
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.
Maybe use a list-comprehension instead? I'm not sure what is the best ;-)
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.
I think that list(map(os.fsencode, pathext))
it's ok.
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.
I did a timeit on both and the list(map())
is slightly faster (on my system), but not much. If it code after this didn't (potentially) iterate over pathext
twice, a generator would be ideal.
Also, the following if
clause uses:
if any(cmd.lower().endswith(ext.lower()) for ext in pathext):
files = [cmd]
Removing the cmd.lower()
from the any
would be a processing optimization, but it's unrelated to this change, so I didn't put it in.
cmd_lower = cmd.lower()
if any(cmd_lower.endswith(ext.lower()) for ext in pathext):
files = [cmd]
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.
LGTM. it's work correctly
Lib/shutil.py
Outdated
|
||
# PATHEXT is necessary to check on Windows. | ||
pathext = os.environ.get("PATHEXT", "").split(os.pathsep) | ||
if isinstance(cmd, bytes): | ||
pathext = list(map(os.fsencode, pathext)) |
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.
I think that list(map(os.fsencode, pathext))
it's ok.
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.
One last remark :-)
@giampaolo: I wrote the initial patch, so I would prefer that someone else would review it. Would mind to review this change please? |
@vstinner no problem; patch LGTM. |
Thanks @giampaolo for the review ;-) |
Thanks @csabella, I merged your PR ;-) |
Original patch by Victor Stinner.
https://bugs.python.org/issue18283