gh-119670: Add force keyword only argument to shlex.quote#148846
gh-119670: Add force keyword only argument to shlex.quote#148846jb2170 wants to merge 17 commits into
force keyword only argument to shlex.quote#148846Conversation
|
|
|
Looks ready (for review again?) to me 😎 |
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
Minor tweaks but please add more tets with quotes inside.
jb2170
force-pushed
the
shlex-quote-force
branch
from
a9e06cd to
e7bf496
Compare
|
Please avoid force-pushing. It's extremely hard to make reviews otherwise. |
jb2170
force-pushed
the
shlex-quote-force
branch
from
e7bf496 to
e3d9b05
Compare
|
Sorry for the force pushes but the 'commit suggestions' commits on GitHub came through as CRLF formatted instead of LF. I don't know if that's a GitHub Microsoft issue,
|
|
I think it's a GH issue because other reviews had the same issue... |
|
Ready |
|
@picnixz 30 day courtesy ping 👀 |
| Hugo van Kemenade in :gh:`148100`.) | ||
|
|
||
|
|
||
| * :mod:`shlex`: |
There was a problem hiding this comment.
Hum. This change will land in Python 3.16 and so Doc/whatsnew/3.16.rst should be modified instead. Moreover, your change adds an entry in Deprecations which is wrong. You should add an entry at https://docs.python.org/dev/whatsnew/3.15.html#improved-modules : add "shlex" sub-section in the Improved modules section.
There was a problem hiding this comment.
Done. I rebased to get the 3.16 whatsnew file. Should I have merged instead of rebase eep
(My thinking was that merging main into this branch would change all the commit hashes for main since this branch, when nope it was the right one ffs 😭 )
There was a problem hiding this comment.
$ git range-diff d206d42..7fa8a7a 6453065..a030aa4 shows everything is the same before and after the rebase
There are propositions to add a single-quote-double-quote switch (pythongh-90630), so to avoid hiccups of people passing `force` as a positional and it being used for the single-double switch, we make kwargs kwargs-only.
Test special cases of strings that don't need quoting, do need quoting, do use `force`, don't use `force` etc. I've tried to be exhaustive.
…to `shlex.force`
Co-authored-by: Bartosz Sławecki <bartosz@ilikepython.com>
The comments for why these tests exist can always be found in the git history Co-authored-by: Bartosz Sławecki <bartosz@ilikepython.com>
Missed from last commit
Intended for another branch shlex-quote-typeerror Co-authored-by: Bartosz Sławecki <bartosz@ilikepython.com>
…view Show off the default behavior as `force=False`
Try not to use don't and we're and should've etc apostrophes.
Add test to ensure string with single quote inside it (without which the string wouldn't need quoting) is quoted even though `force=False`.
Documentation build overview
161 files changed ·
|
|
Really sorry for the rebase ffs, worst PR ever! 😭 |
4 participants
Closes #119670
Supersedes my out-of-date PR #119674
We use
forceinstead ofalwaysas a word more consistent with other programming jargon (like inrm -fthefis for 'force') as discussedCommits
shlex.quote's kwargs kwargs-only. Backwards compatible. There are propositions to add a single-quote-double-quote switch, which I'm ambivalent about, but to future-proof against hiccups of people passingforceas a positional and it ending up being used as the wrong kwarg, we make the kwargs kwargs-only, eg likejson.loads.As mentioned I'm also going to open
an issuea discussion thread aboutshlex.quotereturning"''"for falsey non-str data. This PR and that discussion seem independent of each other, that is this PR doesn't need to be held back until that discussion is addressed.forcekeyword argument #119670📚 Documentation preview 📚: https://cpython-previews--148846.org.readthedocs.build/