Implement chained comparison improvements and related checks #7611
Conversation
Pull Request Test Coverage Report for Build 3231501930
💛 - Coveralls |
Pierre-Sassoulas
added
Enhancement ✨
There was a problem hiding this comment.
Thank you for the work @areveny. There's a lot to review but from what I can see after a surface review it's possible that making the gragh check's code independent from the RefactoringChecker might be a good idea, because 1) There's a lot of code and maintenance would benefit from it being separated 2) It's going to be easier if we want to extends usiing-constant-testsdirectly where it's at instead of refactoring how the checker work to be able to raise any message anywhere.
| "Emitted when items in a boolean condition are all <= or >=" | ||
| "This is equivalent to asking if they all equal.", | ||
| ), | ||
| "R1738": ( |
There was a problem hiding this comment.
Maybe this should raise the existing message for constant comparison using-constant-test (https://pylint.pycqa.org/en/latest/user_guide/messages/warning/using-constant-test.html#using-constant-test-w0125). We might need to relax some constraint on what checker can do, I'm not sure a checker can raise messages it does not define right now.
|
Hi @areveny, just wondering: are the false negatives in the primer result one of the items you were hoping to polish after getting an initial review? |
Pierre-Sassoulas
added
the
Needs take over 🛎️
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #7611 +/- ##
==========================================
- Coverage 95.76% 95.46% -0.30%
==========================================
Files 173 176 +3
Lines 18616 18649 +33
==========================================
- Hits 17827 17803 -24
- Misses 789 846 +57
|
Hi.
This PR adds improvements to
chained-comparisonby processing the chain of comparisons into a graph. It's still a bit unpolished but I would like some higher-level feedback on the direction.It adds two new checks:
impossible-comparisonfor cases when a comparison would always be false, e.g.a > b and b > aora == 5 and a > b and b == 1.comparison-all-equalfor a special cycle case where every comparison allows equality.a >= b >= c and c >= asimplifies toa == b == c.It modifies
chained-comparisonto be more accurate, specifically the casea < b < c and a < cshould be simplified to justa < b < c. It currently is not.It also prints what simplified comparison is.
Unfortunately, this is at the cost of complexity. The algorithm finds the minimum number of comparisons by finding the longest path through the graph of comparisons repeatedly, recalculating all the path lengths after each path is emitted. (I could not get it working by emitting paths in arbitrary order until all nodes are accounted for because of the case above,
a < b < c and a < c.) The overhead of doing so should be limited by the reality that most comparisons tend to have just a few statements.Please let me know what you think. I'm open to big suggestions, but want to share my code for feedback and to keep this moving.
Type of Changes
Description
Closes #5814