| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Removes early return for the special case of an empty string
There was a problem hiding this comment.
Thanks for your interesting change.
Please add tests to Lib/test/test_shlex.py. Also, you can copy the NEWS entry in Doc/whatsnew/3.14.rst, in a new "shlex" sub-section of Improved Modules.
Sorry, something went wrong.
The first assertTrue test checks all strings now start with a single quote, ie they have actually been escaped The second test of assertFalse checks that not all the strings have been escaped when always=False (the default value), since the first, third, and fifth string do not need escaping unless requested.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
There was a problem hiding this comment.
Personally, I'd prefer "force" rather than "always" but that's bike-shedding (using "force" is more common when you want to force a behaviour I think).
Sorry, something went wrong.
| :func:`shlex.quote` now has an *always* keyword argument for forcing | ||
| the escaping of the string passed to it |
There was a problem hiding this comment.
| :func:`shlex.quote` now has an *always* keyword argument for forcing | |
| the escaping of the string passed to it | |
| Allow :func:`shlex.quote` to unconditionally escape its input | |
| via the keyword-only argument *always*. |
Sorry, something went wrong.
| * Add keyword-only argument of *always* to :func:`shlex.quote` for forcing | ||
| the escaping of the string passed to it. |
There was a problem hiding this comment.
* Allow :func:`shlex.quote` to unconditionally escape its input
via ``shlex.quote(string, always=True)``.
(Contributed by YOUR_NAME_OR_GITHUB_NICKNAME in :gh:`119670`.)
Sorry, something went wrong.
| If the *always* keyword argument is set to ``True`` then the string *s* | ||
| will always be escaped, even in the absence of special characters. This is | ||
| nice for uniformity, for example when escaping a list of strings. |
There was a problem hiding this comment.
| If the *always* keyword argument is set to ``True`` then the string *s* | |
| will always be escaped, even in the absence of special characters. This is | |
| nice for uniformity, for example when escaping a list of strings. | |
| If *always* is ``True``, then the string *s* is | |
| unconditionally escaped, even in the absence of | |
| special characters. This is typically useful for | |
| uniformly escaping a list of strings: |
Sorry, something went wrong.
Oh, I also like force=True. What do you think @jb2170? |
Sorry, something went wrong.
I could be swayed; force seems like a conventional word within the programming world, eg 'rm -rf'. But that is then superseded by the following:
Changing the behaviour to always quote its input would be ideal in my eyes! It requires tweaking a couple of the tests in Lib/test/test_shlex.py but I will commit those. At this point however I question "who would ever want quote(s, force=False)?". We could get rid of the force kwarg, returning to the function's existing mono-parameter signature, and reduce the function body down to just def quote(s):
return "'" + s.replace("'", "'\"'\"'") + "'"This simplifies quote to the one-liner that it should be, and it does-what-it-says-on-the-tin. Oh and the re import can be removed, since that's the only place it's used! 😄 Perhaps my main issue is that quote doesn't always quote, its behaviour quite unpredictable from the function name alone, relying on a footgun regex.If we were to go ahead with this simplification the only question is "is there anyone out there relying on basic strings not being quoted by quote?". I would guess no-one would care, paraphrasing the discussion on PEP 475's backwards compatibility "The authors of this (PEP) don’t think that such applications exist" I'll push a commit, see what you think... 👀 😃 |
Sorry, something went wrong.
|
I only asked about the parameter name force instead of always, not to enable it by default, sorry. |
Sorry, something went wrong.
|
Could you:
|
Sorry, something went wrong.
|
It seems like @jb2170 is inactive for one year. Sadly, I suggest closing the PR and the related issue. |
Sorry, something went wrong.
|
I'm still here, I'll give it a second look 😅 |
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
|
I'll do a fresh rebase / branch / PR since this PR is very much behind main. Sorry for abandoning it a while! It's coming along on my branch shlex-quote-force. I suggest force to be an opt-in keyword-only argument for shlex.quote. 🙂 I want to talk about this behavior of shlex.quote too as it caught my attention when I was editing the function. I think the returning of "''" for falsey non-str data isn't right. I've put some commits on another branch shlex-quote-typeerror. I'll have a sleep on it. Discussion thread open here I'll check my work and open a PR for shlex-quote-force later today. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Addresses #119670
📚 Documentation preview 📚: https://cpython-previews--119674.org.readthedocs.build/