Remove redundant if check from argparser file.#8766
Conversation
Remove redundant if check from optional argument function in argparser.
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. When your account is ready, please add a comment in this pull request You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
|
signed the CLA, Please remove the |
| option_strings.append(option_string) | ||
| if option_string[0] in self.prefix_chars: | ||
| if len(option_string) > 1: | ||
| if option_string[1] in self.prefix_chars: |
There was a problem hiding this comment.
I don't think these checks are redundant.
Consider self.prefix_chars = {'-'}
The left hand side requires '--foo' whereas your updated code incorrectly allows X-foo
I do think these conditions could be combined, perhaps something like:
if (
option_string[0] in self.prefix_chars and
len(option_string) > 1 and
option_string[1] in self.prefix_chars
):
...There was a problem hiding this comment.
Please check the code from line no 1463. It checks for the condition option_string[0] in self.prefix_chars.
There was a problem hiding this comment.
Ah indeed! Maybe use elif so that's more clear at a glance?
There was a problem hiding this comment.
I can do this.
if not option_string[0] in self.prefix_chars:
# Some code.
raise
else:
option_strings.append(option_string)
if len(option_string) > 1 and option_string[1] in self.prefix_chars:
long_option_strings.append(option_string)
Or the current code is also fine.
There was a problem hiding this comment.
@asottile This change looks good. No need for else. This looks cleaner. Please review.
asottile
left a comment
asottile
left a comment
There was a problem hiding this comment.
yeah you;re right, seems fine -- didn't notice the statement in the middle so elif would be way more work than necessary.
|
@asottile How and when will this Pull request be approved and merged to master? |
|
No idea, I'm just a rando |
|
@bedevere-bot Please someone review. |
|
@benjaminp @merwok Please review the PR. |
* master: (599 commits) Docs: Improved phrasing (pythonGH-14069) Remove redundant if check from optional argument function in argparse. (pythonGH-8766) bpo-37289: Add a test for if with ifexpr in the peephole optimiser to detect regressions (pythonGH-14127) Update What's New in Python 3.9 (pythonGH-14253) bpo-36511: Improve ARM32 buildbot scripts (pythonGH-14251) bpo-37151: remove _PyCFunction_FastCallDict (pythonGH-14269) Fix typo, 'widger' -> 'widget', in idlelib/tree.py (pythonGH-14263) Fix bpo number in News file. (pythonGH-14260) bpo-37342: Fix the incorrect nb_index's type in typeobj documentation (pythonGH-14241) Update What's New in Python 3.8 (pythonGH-14239) bpo-36710: Use tstate in pylifecycle.c (pythonGH-14249) Add missing single quote in io.TextIOWrapper.reconfigure documentation (pythonGH-14246) bpo-36511: Add buildbot scripts and fix tests for Windows ARM32 buildbot (pythonGH-13454) bpo-37333: Ensure IncludeTkinter has a value (pythonGH-14240) bpo-37331: Clarify format of socket handler messages in the documentation. (pythonGH-14234) bpo-37258: Not a bug, but added a unit test and updated documentation. (pythonGH-14229) bpo-36710: Remove PyImport_Cleanup() function (pythonGH-14221) Fix name of '\0'. (pythonGH-14222) bpo-36710: Add tstate parameter in import.c (pythonGH-14218) Document typing.ForwardRef (pythonGH-14216) ...
6 participants
Remove redundant if check from optional argument function in argparser.