nodeenv icon indicating copy to clipboard operation
nodeenv copied to clipboard

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

Open musicinmybrain opened this issue 2 years ago • 4 comments

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.

musicinmybrain avatar Oct 23 '23 13:10 musicinmybrain

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

neutrinoceros avatar Nov 04 '23 12:11 neutrinoceros

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.

musicinmybrain avatar Nov 04 '23 13:11 musicinmybrain

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.

neutrinoceros avatar Nov 04 '23 13:11 neutrinoceros

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

rschwebel avatar Dec 19 '23 22:12 rschwebel

Thanks!

ekalinin avatar May 28 '24 18:05 ekalinin