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

On Python 3.3+, replace pipes.quote with shlex.quote #342

Merged
merged 2 commits into from
May 28, 2024

Conversation

musicinmybrain
Copy link
Contributor

The pipes.quote() function was undocumented, and the pipes module was deprecated in Python 3.11 and will be removed in Python 3.13.

Fixes #341.

This is a one-to-one replacement of pipes.quote with shlex.quote; I did not attempt to audit the shell escaping for correctness and safety.

The pipes.quote() function was undocumented, and the pipes module was
deprecated in Python 3.11 and will be removed in Python 3.13.

Fixes ekalinin#341.
@neutrinoceros
Copy link

neutrinoceros commented Nov 4, 2023

I would suggest to switch to if/else for version detection because that pattern can be automatically cleaned with modern tooling (pyupgrade, ruff), while a try/except cannot (to the best of my knowledge)
See #344

It was pointed out that pyupgrade can handle this better than
try/except.
@musicinmybrain
Copy link
Contributor Author

I would suggest to switch to if/else for version detection because that pattern can be automatically cleaned with modern tooling (pyupgrade, ruff), while a try/except cannot (to the best of my knowledge)

That’s an interesting point. I tested it to be sure. With pyupgrade --py38-plus nodeenv.py, we would see:

 try:
     from shlex import quote as _quote  # Python 3.3+
 except ImportError:
-    from pipes import quote as _quote  # Python 2.7
+    from shlex import quote as _quote  # Python 2.7
 import platform

which should point out the need remove the try/except to upgrade to a human reviewer, but doesn’t fully handle it by itself. (This makes sense: there can be a lot of surprising and exotic reasons for an ImportError in general.)

If I switch to:

if sys.version_info < (3, 3):
    from pipes import quote as _quote
else:
    from shlex import quote as _quote

I get:

 import argparse
 import subprocess
 import tarfile
-if sys.version_info < (3, 3):
-    from pipes import quote as _quote
-else:
-    from shlex import quote as _quote
+from shlex import quote as _quote
 import platform
 import zipfile
 import shutil

A human reviewer might still choose to change this back to import shlex and call shlex.quote for consistency with the overall style.

I did notice that if I slice version_info, e.g. testing sys.version_info[:2], it breaks pyupgrade’s heuristic and the if/else is not removed.

If ruff knows how to do “upgrades” like this, I don’t know how to ask for it.


Anyway, it seems like this basically supports your suggestion. I’ve added a commit to switch to an if/else for now, but I’m curious what @ekalinin thinks.

@neutrinoceros
Copy link

If ruff knows how to do “upgrades” like this, I don’t know how to ask for it.

ruff's UP rule set is a reimplementation of pyupgrade, which includes the refactor you just illustrated. The difference is that ruff is also able to read metadata from pyproject.toml, so it "just knows" what refactors can be safely applied.

@rschwebel
Copy link
Contributor

The checks are broken due to #347, this PR is not the culprit.

@ekalinin ekalinin merged commit c1dffc5 into ekalinin:master May 28, 2024
1 of 7 checks passed
@ekalinin
Copy link
Owner

Thanks!

ekalinin added a commit that referenced this pull request May 28, 2024
ekalinin added a commit that referenced this pull request May 28, 2024
* fix tests after #342

* add coverage files into gitignore
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.

Remove usage of pipes
4 participants