Add improve-conditionals check in the CodeStyle extension #10600
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdce8p
added
Enhancement ✨
cdce8p
commented
Sep 29, 2025
Comment on lines
78
to
85
| "R6106": ( | ||
| "Rewrite conditional expression to '%s'", | ||
| "improve-conditionals", | ||
| "Rewrite negated if expressions to improve readability.", | ||
| { | ||
| # "default_enabled": False, | ||
| }, | ||
| ), |
There was a problem hiding this comment.
This could probably be improved. Open to suggestions.
We could also change the checker name if someone has a good idea.
Besides that, should the checker be disabled by default even in the optional CodeStyle extension?
There was a problem hiding this comment.
Either that or it should be a pylint plugin. I regret including magic-number-comparison because then Ruff implemented it and Ruff does not take the default disable into account and it makes pylint look really opinionated and unreasonable.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (89.69%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #10600 +/- ##
==========================================
- Coverage 95.95% 95.92% -0.04%
==========================================
Files 176 176
Lines 19476 19525 +49
==========================================
+ Hits 18689 18730 +41
- Misses 787 795 +8
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Writing the doc example with animal made me reconsider if the suggestion is always more readable. I guess when there's a double negation not x or not y, then yes, not (x and y) is better for not x or y >= 0 then not (x and y < 0) is slightly better, otherwise I don't know. The primer also point to an especially opinionated check that could be an external plugin. Let us hear from others.
Comment on lines
78
to
85
| "R6106": ( | ||
| "Rewrite conditional expression to '%s'", | ||
| "improve-conditionals", | ||
| "Rewrite negated if expressions to improve readability.", | ||
| { | ||
| # "default_enabled": False, | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Either that or it should be a pylint plugin. I regret including magic-number-comparison because then Ruff implemented it and Ruff does not take the default disable into account and it makes pylint look really opinionated and unreasonable.
Comment on lines
1
to
4
| def func(expr, node_cls): | ||
| # +1:[improve-conditionals] | ||
| if not isinstance(expr, node_cls) or expr.attrname != "__init__": | ||
| ... |
There was a problem hiding this comment.
Suggested change
| def func(expr, node_cls): | |
| # +1:[improve-conditionals] | |
| if not isinstance(expr, node_cls) or expr.attrname != "__init__": | |
| ... | |
| def is_platypus(animal): | |
| # The platypus is both the only mammal with a beak and without nipples. | |
| # +1:[improve-conditionals] | |
| return animal.is_mammal() and (not animal.has_nipples() or animal.has_beak) |
There was a problem hiding this comment.
Your example wouldn't actually raise the message. Yes, it could be inverted but so far I've chosen not to emit it if we'd need to add not, only if it could be removed / moved before the BoolOp. I.e. it would only be emitted for
# bad
x and (not y or not z)
# good
x and not (y and z)There was a problem hiding this comment.
Ok the checker is a lot less opinionated than I thought then : good.
I had to do some extensive phenology for this one, turn out uniquely identifying an animal by lack of a characteristic is not so easy.
Suggested change
| def func(expr, node_cls): | |
| # +1:[improve-conditionals] | |
| if not isinstance(expr, node_cls) or expr.attrname != "__init__": | |
| ... | |
| def is_penguin(animal): | |
| # Penguins are the only flightless, kneeless sea birds | |
| # +1:[improve-conditionals] | |
| return animal.is_seabird() and (not animal.can_fly() or not animal.has_visible_knee()) |
There was a problem hiding this comment.
Ok the checker is a lot less opinionated than I thought then : good.
I went through all of the warning for Home Assistant. They looked quite good. There was just one common pattern I don't think the checker should emit a warning for.
# is x between 0 and 100
if x > 0 or x < 100:
...Addressed that one in b08beb4
pylint/extensions/private_import.py
Outdated
| bool(name) | ||
| and name[0] == "_" | ||
| and (len(name) <= 4 or name[1] != "_" or name[-2:] != "__") | ||
| and not (len(name) > 4 and name[:2] == "__" and name[-2:] == "__") |
There was a problem hiding this comment.
The condition was done with great care about performance here, I'm not sure changing the order without benchmark is advisable.
There was a problem hiding this comment.
Are they any different though? Let's consider some cases
Returning True
To return True with the "old" code, any one of the conditions had to evaluate to true and or short-circuits the conditional. With the "new" one it's just the inverse. Any one condition has to eval to false and and short-circuits as well. That's fine as all comparisons are inverted itself.
Returning False
With or all conditions had to be checked to make sure none actually returned true. Same with and. It will only return false if all conditions eval to true.
--
Am I missing something here? Tbh I had to think hard about it and even thought they aren't identical for a few minutes. In the end however, they should evaluate almost exactly the same.
--
I reverted the same change to the second check with name[:2] (instead of name[1]) in 7e4bb03.
There was a problem hiding this comment.
I looked at it too fast and thought that the prior check for first char is _ was removed, but it's not. We already checked the first character so name[:2] == "__" should stay name[1] == "_". And in this case it become equivalent.
Co-authored-by: Pierre Sassoulas <[email protected]>
cdce8p
force-pushed
the
check-improved-conditionals
branch
from
840dfbf to
7e4bb03
Compare
This comment has been minimized.
This comment has been minimized.
cdce8p
commented
Oct 2, 2025
There was a problem hiding this comment.
I think this check is ready now. Yes, it's a bit opinionated but that could even be a positive thing. In the end it'll be disabled by default in an optional checker.
At last I just want to highlight one of the cases I found in Home Assistant. There aren't a lot of them but sometimes it can be just art 🧑🎨
# current
if (
not isinstance(data, dict)
or "eventType" not in data
or data["eventType"] != "changeReport"
or "eventVersion" not in data
or data["eventVersion"] != "1"
or "context" not in data
or not isinstance(data["context"], dict)
or "deviceType" not in data["context"]
or "deviceMac" not in data["context"]
):
return# with 'and' instead of 'or' it gets obvious that this might be a great place for match
if not (
isinstance(data, dict)
and "eventType" in data
and data["eventType"] == "changeReport"
and "eventVersion" in data
and data["eventVersion"] == "1"
and "context" in data
and isinstance(data["context"], dict)
and "deviceType" in data["context"]
and "deviceMac" in data["context"]
):
returnmatch data:
case {
"eventType": "changeReport",
"eventVersion": "1",
"context": {"deviceType": _, "deviceMac": _},
}:
pass
case _:
return
Comment on lines
1
to
4
| def func(expr, node_cls): | ||
| # +1:[improve-conditionals] | ||
| if not isinstance(expr, node_cls) or expr.attrname != "__init__": | ||
| ... |
There was a problem hiding this comment.
Ok the checker is a lot less opinionated than I thought then : good.
I went through all of the warning for Home Assistant. They looked quite good. There was just one common pattern I don't think the checker should emit a warning for.
# is x between 0 and 100
if x > 0 or x < 100:
...Addressed that one in b08beb4
pylint/extensions/private_import.py
Outdated
| bool(name) | ||
| and name[0] == "_" | ||
| and (len(name) <= 4 or name[1] != "_" or name[-2:] != "__") | ||
| and not (len(name) > 4 and name[:2] == "__" and name[-2:] == "__") |
There was a problem hiding this comment.
Are they any different though? Let's consider some cases
Returning True
To return True with the "old" code, any one of the conditions had to evaluate to true and or short-circuits the conditional. With the "new" one it's just the inverse. Any one condition has to eval to false and and short-circuits as well. That's fine as all comparisons are inverted itself.
Returning False
With or all conditions had to be checked to make sure none actually returned true. Same with and. It will only return false if all conditions eval to true.
--
Am I missing something here? Tbh I had to think hard about it and even thought they aren't identical for a few minutes. In the end however, they should evaluate almost exactly the same.
--
I reverted the same change to the second check with name[:2] (instead of name[1]) in 7e4bb03.
|
🤖 Effect of this PR on checked open source code: 🤖 Effect on home-assistant:
This comment was truncated because GitHub allows only 65536 characters in a comment. This comment was generated for commit 87e5786 |
| @@ -0,0 +1,3 @@ | |||
| Add :ref:`improve-conditionals` check to the Code Style extension. | |||
There was a problem hiding this comment.
Suggested change
| Add :ref:`improve-conditionals` check to the Code Style extension. | |
| Add :ref:`improve-conditionals` check to the Code Style extension to suggest `not(x and y)` instead of `not x or not y` in order to facilitate the detection of match case refactors. The code style extension must be enabled and `improve-conditionals` itself needs to be explicitely enabled. |
| ), | ||
| "R6106": ( | ||
| "Rewrite conditional expression to '%s'", | ||
| "improve-conditionals", |
There was a problem hiding this comment.
The name is a little vague. What about consider-factorizing-not, 'or-with-double-negation'... ? I'm on mobile but asking a llm about it might give the least surprising name for it.
|
The exemple you give does make sense. What if we were raising only strictly above 2 negated conditions ? It's not the same purpose than the initial idea but it would make thé check a lot less opinionated |
|
Or say we add an option for the threshold of negated conditions, and activate the message by default with a value of 4 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Followup to #10581
Suggest rewriting conditional expressions
I've added the check in the
CodeStyleextension. It might even make sense to disable it by default.