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 incorrect consider-using-ternary when condition is inferable as False #5227

Merged
merged 3 commits into from Oct 29, 2021

Conversation

areveny
Copy link
Contributor

@areveny areveny commented Oct 28, 2021

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Closes #5200

See that issue for my triage/explanation of the fix.

@coveralls
Copy link

coveralls commented Oct 28, 2021

Pull Request Test Coverage Report for Build 1399479523

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.346%

Totals Coverage Status
Change from base Build 1389963373: 0.0%
Covered Lines: 13707
Relevant Lines: 14684

πŸ’› - Coveralls

… as False

* Properly infer the condition in old ternary statements and suggest ``simplify-boolean-expression`` over ``consider-using-ternary`` if it is False

* Integration test for this behavior
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.

Thanks, I think this is a great bug fix. I just have one small remark.

@@ -1396,7 +1396,7 @@ def visit_return(self, node: nodes.Return) -> None:
if inferred_truth_value in (None, astroid.Uninferable):
truth_boolean_value = True
else:
truth_boolean_value = truth_value.bool_value()
truth_boolean_value = inferred_truth_value.bool_value() # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

What is the mypy error here ? Any way we can fix this ?

Copy link
Contributor Author

@areveny areveny Oct 29, 2021

Choose a reason for hiding this comment

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

Below is the output I get running mypy with tox on my laptop. The last three errors I don't get on my desktop, but I haven't delved into what the differential is.

I assumed the first error was ignorable because logically inferred_truth_value should never be None at this line. I'm not very familiar with mypy so I should have asked proactively but if you have a pointer of what to look at here I can look into it more.

pylint/checkers/refactoring/refactoring_checker.py:1399: error: Item "None" of "Optional[Any]" has no attribute "bool_value"
tests/lint/test_pylinter.py:7: error: unused "type: ignore" comment
tests/test_pylint_runners.py:8: error: unused "type: ignore" comment
tests/test_self.py:59: error: unused "type: ignore" comment
Found 4 errors in 4 files (checked 207 source files)

Copy link
Member

Choose a reason for hiding this comment

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

If you modify L1396, you could remove the type: ignore here.

if inferred_truth_value is None or inferred_truth_value == astroid.Uninferable:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip! I'll brush up on handling mypy output more too.

Copy link
Member

Choose a reason for hiding this comment

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

mypy can't really handle in conditions unfortunately. More explicit ones are a bit easier, thus the suggestion works.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.12.0 milestone Oct 29, 2021
@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Oct 29, 2021
@@ -1396,7 +1396,7 @@ def visit_return(self, node: nodes.Return) -> None:
if inferred_truth_value in (None, astroid.Uninferable):
truth_boolean_value = True
else:
truth_boolean_value = truth_value.bool_value()
truth_boolean_value = inferred_truth_value.bool_value() # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

If you modify L1396, you could remove the type: ignore here.

if inferred_truth_value is None or inferred_truth_value == astroid.Uninferable:

ChangeLog Outdated Show resolved Hide resolved
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

Nice merge request again @areveny !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong ternary suggested
4 participants