Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.4k
Remove redundant if check from argparser file.#8766
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Remove redundant if check from optional argument function in argparser.
the-knights-who-say-ni commentedAug 14, 2018
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 thePSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username onbugs.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 followthe steps outlined in the CPython devguide to rectify this issue. When your account is ready, please add a comment in this pull request You cancheck yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
sp1rs commentedAug 14, 2018
signed the CLA, Please remove the |
| option_strings.append(option_string) | ||
| ifoption_string[0]inself.prefix_chars: | ||
| iflen(option_string)>1: | ||
| ifoption_string[1]inself.prefix_chars: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think these checks are redundant.
Considerself.prefix_chars = {'-'}
The left hand side requires'--foo' whereas your updated code incorrectly allowsX-foo
I do think these conditions could be combined, perhaps something like:
if (option_string[0]inself.prefix_charsandlen(option_string)>1andoption_string[1]inself.prefix_chars): ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please check the code from line no 1463. It checks for the conditionoption_string[0] in self.prefix_chars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah indeed! Maybe useelif so that's more clear at a glance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can do this.
if not option_string[0] in self.prefix_chars: # Some code. raiseelse: 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@asottile This change looks good. No need forelse. This looks cleaner. Please review.
asottile left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yeah you;re right, seems fine -- didn't notice the statement in the middle soelif would be way more work than necessary.
sp1rs commentedAug 24, 2018
@asottile How and when will this Pull request be approved and merged to master? |
asottile commentedAug 24, 2018
No idea, I'm just a rando |
sp1rs commentedNov 29, 2018
@bedevere-bot Please someone review. |
sp1rs commentedJun 20, 2019
@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) ...
Remove redundant if check from optional argument function in argparser.