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.

Sign up for GitHub

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

Jump to bottom

Fix incorrect consider-using-ternary when condition is inferable as False #5227

Merged
merged 3 commits into from
Oct 29, 2021
Merged

Fix incorrect consider-using-ternary when condition is inferable as False #5227

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9bcdf07
Fix incorrect ``consider-using-ternary`` when condition is inferrable…
areveny Oct 28, 2021
99b0de6
Refactor logic to null check more explicitly and avoid a mypy error
areveny Oct 29, 2021
6891588
Make documentation more concise
areveny Oct 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ Release date: TBA
..
Put new features here and also in 'doc/whatsnew/2.12.rst'

* Fix proposing ``consider-using-ternary`` incorrectly over ``simplify-boolean-expression``
when the condition can be inferred as False
areveny marked this conversation as resolved.
Show resolved Hide resolved

Closes #5200

* Fix exception when pyreverse parses ``property function`` of a class.

* Add an optional extension ``consider-using-any-or-all`` : Emitted when a ``for`` loop only
Expand Down
5 changes: 5 additions & 0 deletions doc/whatsnew/2.12.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ Extensions
Other Changes
=============

* Fix proposing ``consider-using-ternary`` incorrectly over ``simplify-boolean-expression``
when the condition can be inferred as False

Closes #5200

* Fix exception when pyreverse parses ``property function`` of a class.

* Improve and flatten ``unused-wildcard-import`` message
Expand Down
2 changes: 1 addition & 1 deletion pylint/checkers/refactoring/refactoring_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited

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.


if truth_boolean_value is False:
message = "simplify-boolean-expression"
Expand Down
6 changes: 6 additions & 0 deletions tests/functional/t/ternary.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,9 @@ def func4():
""""Using a Name as a condition but still emits"""
truth_value = 42
return condition and truth_value or false_value # [consider-using-ternary]


def func5():
""""Using a Name that infers to False as a condition does not emit"""
falsy_value = False
return condition and falsy_value or false_value # [simplify-boolean-expression]
1 change: 1 addition & 0 deletions tests/functional/t/ternary.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ consider-using-ternary:19:0::Consider using ternary ('greater' if SOME_VALUE1 >
consider-using-ternary:20:0::Consider using ternary ('both' if SOME_VALUE2 > 4 and SOME_VALUE3 else 'not')
simplify-boolean-expression:23:0::Boolean expression may be simplified to SOME_VALUE2
consider-using-ternary:33:4:func4:Consider using ternary (truth_value if condition else false_value)
simplify-boolean-expression:39:4:func5:Boolean expression may be simplified to false_value