Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix false positive for superfluous-parens for return (a or b) in iterable #5964

Merged

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
🐛 Bug fix

Description

Fix false positive for superfluous-parens for patterns like return (a or b) in iterable.

Closes #5803

@jacobtylerwalls jacobtylerwalls changed the title Fix false positive for superfluous-parens for return (a, b) in iterable Fix false positive for superfluous-parens for return (a or b) in iterable Mar 25, 2022
@coveralls
Copy link

coveralls commented Mar 25, 2022

Pull Request Test Coverage Report for Build 2044359428

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • 103 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.01%) to 94.184%

Files with Coverage Reduction New Missed Lines %
pylint/reporters/multi_reporter.py 1 98.21%
pylint/reporters/reports_handler_mix_in.py 2 94.74%
pylint/extensions/typing.py 4 97.08%
pylint/reporters/ureports/nodes.py 4 94.52%
pylint/utils/utils.py 13 86.9%
pylint/checkers/refactoring/refactoring_checker.py 15 98.25%
pylint/checkers/utils.py 29 95.97%
pylint/checkers/variables.py 35 96.49%
Totals Coverage Status
Change from base Build 2039774678: 0.01%
Covered Lines: 15336
Relevant Lines: 16283

💛 - Coveralls

Comment on lines +388 to +391
if keyword_token == "in":
# This special case was added in https://github.com/PyCQA/pylint/pull/4948
# but it could be removed in the future. Avoid churn for now.
return
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielNoord this is here to preserve the special case added in #4948, but when I looked into it, I had trouble figuring out why it was deemed a bugfix. (Looks like unnecessary parentheses to me!) Do you recall the thought behind it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still pass:
D = [x for x in ((3, 4) if 1 > 0 else ((5, 6)))]

See #4907 (comment)

I think sometimes people want to create inner tuples and this was added so we don't flag those.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it still passes now, but I'm essentially suggesting we later revert #4948.

I think sometimes people want to create inner tuples and this was added so we don't flag those.

I'm not sure what inner tuple means here. ((5,6),) is a tuple of a tuple. ((5,6)) is superfluous parentheses, so it could just be disabled if people really prefer the extra parentheses. But I think maybe there's a nuance here I'm not getting?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still pass:
D = [x for x in ((3, 4) if 1 > 0 else ((5, 6)))]

(This test is still in the suite)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm perhaps I was wrong to re-allow those.

I thought there might be use cases where you would want to create Tuple[Tuple[Tuple[int, ...]]] instead of Tuple[int, ...]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but that's (((5,),),) rather than (((5,))).

Parentheses increase readability, so I understand why people use them, belt and suspenders and all, but I'm just saying those folks might not want the check enabled if they're in the practice of adding them.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm fine with reverting then. But we should check #4907 for any legitimate use cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll capture this discussion on #5361

@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Mar 25, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.1 milestone Mar 25, 2022
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'm thinking the changelog should be in 2.13.1 in ChangeLog and in 2.14 in whatsnew if we backport this but let me know what you think. (edit: resolved in another discussion)

ChangeLog Outdated Show resolved Hide resolved
doc/whatsnew/2.14.rst Outdated Show resolved Hide resolved
tests/functional/s/superfluous_parens.py Show resolved Hide resolved
Comment on lines +388 to +391
if keyword_token == "in":
# This special case was added in https://github.com/PyCQA/pylint/pull/4948
# but it could be removed in the future. Avoid churn for now.
return
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still pass:
D = [x for x in ((3, 4) if 1 > 0 else ((5, 6)))]

See #4907 (comment)

I think sometimes people want to create inner tuples and this was added so we don't flag those.

@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.13.1, 2.13.2 Mar 26, 2022
@DanielNoord
Copy link
Collaborator

@jacobtylerwalls Do we understand the coverage report?

@jacobtylerwalls
Copy link
Member Author

@jacobtylerwalls Do we understand the coverage report?

From the edit history of the comment the lines lost coverage when I merged main 10 hours ago, so ... yes? :D (I don't really have a mental model of how Coveralls handles merges into PR branches.)

@Pierre-Sassoulas Pierre-Sassoulas merged commit 96d7e89 into pylint-dev:main Mar 26, 2022
@jacobtylerwalls jacobtylerwalls deleted the superfluous-paren-fix branch March 26, 2022 22:35
@Pierre-Sassoulas Pierre-Sassoulas removed the Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer label Mar 27, 2022
Pierre-Sassoulas added a commit that referenced this pull request Mar 27, 2022
…terable` (#5964)

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False alarm for superfluous-parens
4 participants