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

Use subprocess instead of os.system #1477

Open
wants to merge 3 commits into
base: stable
Choose a base branch
from
Open

Conversation

jwodder
Copy link
Contributor

@jwodder jwodder commented Feb 22, 2020

closes #1026, closes #1470, closes #1476

click/termui.py Outdated Show resolved Hide resolved
click/_compat.py Outdated Show resolved Hide resolved
click/_compat.py Outdated Show resolved Hide resolved
@davidism davidism changed the title [#1470,#1476] Use subprocess instead of os.system Use subprocess instead of os.system Feb 22, 2020
click/_compat.py Outdated Show resolved Hide resolved
@davidism
Copy link
Member

Master is now Python 3 only. This needs to be rebased, and compat for shutil.which can be removed.

#1543 reverts the previous shlex.quote pr because of an issue with calling quote on a command that might actually be a command and options, like less -FRSX (and also an issue with quoting on Windows, which is sidestepped here). Unfortunately, just calling shlex.split on the command first will fail on Windows unless slashes in paths are double escaped:

>>> shlex.split("C:\\path\\to\\exe /test other\\path")
['C:pathtoexe', '/test', 'otherpath']
>>> shlex.split("C:\\path\\to\\exe /test other\\path".replace("\\", "\\\\"))
['C:path\\to\\exe', '/test', 'other\\path']
>>> shlex.split("C:\\path\\to\\exe /test other\\path", posix=False)
['C:path\\to\\exe', '/test', 'other\\path']

Technically that last one with posix=False is not correct, as the argument refers to non-Posix shells on Unix, not completely different platforms like cmd on Windows. Either solution would probably be fine for the limited case here, although I'd lean towards replace since it's more clear what's being special-cased.

@AndreasBackx AndreasBackx changed the base branch from main to stable October 26, 2024 14:06
@@ -377,16 +378,16 @@ def pager(generator: t.Iterable[str], color: t.Optional[bool] = None) -> None:
if os.environ.get("TERM") in ("dumb", "emacs"):
return _nullpager(stdout, generator, color)
if WIN or sys.platform.startswith("os2"):
return _tempfilepager(generator, "more <", color)
if hasattr(os, "system") and os.system("(less) 2>/dev/null") == 0:
return _tempfilepager(generator, "more", color, pipe=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed _pipepager exists now. Should we not use that instead here and remove the pipe argument I introduced?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use subprocess instead of os.system click.edit() Support Editor Paths Containing Spaces
3 participants