| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
* Simplify the code on Posix * Fix the code on Windows. * Use absolute paths in tests.
| else: | ||
| args = list(args) | ||
| args[0] = os.fsdecode(args[0]) | ||
| args = list2cmdline(args) |
There was a problem hiding this comment.
This needs to be addressed in list2cmdline to be consistent with how POSIX converts every element of args via PyUnicode_FSConverter. For example:
if isinstance(args, bytes):
args = os.fsdecode(args)
elif isinstance(args, os.PathLike):
if shell:
raise ValueError('PathLike args is not allowed when '
'shell is true.')
else:
args = [args]
if not isinstance(args, str):
args = list2cmdline(args)
if executable is not None:
executable = os.fsdecode(executable)
Then in list2cmdline, decode the items via os.fsdecode :
for arg in seq:
if not isinstance(arg, str):
arg = os.fsdecode(arg)
Sorry, something went wrong.
There was a problem hiding this comment.
This does look like it would be nicer, allowing more mixing and matching of types.
Sorry, something went wrong.
There was a problem hiding this comment.
@eryksun 's comment is related to the old version of the PR. I addressed it, except that that I decided to reject bytes args on Windows with shell=True as it is ambiguous and is not actually needed. It is enough to have a difference in interpreting str args.
Sorry, something went wrong.
|
|
||
| if executable is not None: | ||
| executable = os.fsdecode(executable) | ||
|
|
There was a problem hiding this comment.
I still think bytes and pathlike should be supported for every item of args. The POSIX implementation doesn't limit this to args[0] (though not documented), and I think that's proper. A script shouldn't have to special-case calling os.fsdecode for a pathlike argument. That's inefficient for POSIX, for which os.fsencode would be better. While os.fspath would be efficient, it's insufficient because it might return bytes on Windows.
Sorry, something went wrong.
There was a problem hiding this comment.
The POSIX implementation accepts also bytes as args and as any item of args. If allow path-like arguments, we should consider also adding support of bytes arguments and program name.
Sorry, something went wrong.
|
You convinced me. I made bytes and pathlike be supported for every item of args. But bytes as a single argument is not supported because this case is ambiguous. It is not clear how to interpret it: as a full command line (as single str value) or as a program file name (as single path-like value). |
Sorry, something went wrong.
|
@gpshead, could you please take a look? |
Sorry, something went wrong.
| if not isinstance(args, str): | ||
| if isinstance(args, str): | ||
| pass | ||
| elif isinstance(args, bytes): |
There was a problem hiding this comment.
The elif chain could be simplified to have less repeated code:
elif isinstance(args, (bytes, os.PathLike)):
if shell:
raise TypeError(f'{type(args)} args not allowed when shell=True on Windows')`
args = list2cmdline([args])
Sorry, something went wrong.
There was a problem hiding this comment.
Error messages should be different. Path-like args is not allowed on all platforms, bytes args is not allowed only on Windows.
Sorry, something went wrong.
| args = [args] | ||
| elif isinstance(args, os.PathLike): | ||
| if shell: | ||
| raise TypeError('path-like args is not allowed when ' |
There was a problem hiding this comment.
for consistency of error messages with my suggestion above I think TypeError(f'{type(args)} args is not allowed when ' ...). It is good for that to render as <class os.PathLike> as that is less confusing to the error message reader than "path-like" which they might be more likely to mis-interpret in English as suggesting anything that looks like a path within a string.
Sorry, something went wrong.
| self.assertEqual(cp.returncode, 33) | ||
|
|
||
| def test_run_with_pathlike_path(self): | ||
| # bpo-31961: test run(pathlike_object) |
There was a problem hiding this comment.
I like to make the first line descriptive comment a docstring. it shows up nicely in verbose testing mode.
Sorry, something went wrong.
| *cwd* parameter accepts a :term:`path-like object`. | ||
| *cwd* parameter accepts a :term:`path-like object` on POSIX. | ||
|
|
||
| .. versionchanged:: 3.7 |
There was a problem hiding this comment.
FYI - these 3.6 and 3.7 versionchanged updates should be made to the docs today to clarify the existing behavior on their own (and backported to at least the 3.7 docs)
Sorry, something went wrong.
| else: | ||
| args = list(args) | ||
| args[0] = os.fsdecode(args[0]) | ||
| args = list2cmdline(args) |
There was a problem hiding this comment.
This does look like it would be nicer, allowing more mixing and matching of types.
Sorry, something went wrong.
…thon into subprocess-pathlike
|
#13628 depends on this PR for Windows tests green (we can skip Windows, sure. But I honestly prefer to fix upstream :) |
Sorry, something went wrong.
|
I think that I addressed all review comments, but if I missed something it can be fixed in the other PR (even after beta1). |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
https://bugs.python.org/issue31961