Add additional checks for match class patterns #10587
Conversation
cdce8p
force-pushed
the
additional-match-checks
branch
from
f6d8d9d to
42b289d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10587 +/- ##
=======================================
Coverage 95.95% 95.95%
=======================================
Files 176 176
Lines 19455 19469 +14
=======================================
+ Hits 18668 18682 +14
Misses 787 787
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
| inferred = safe_infer(node.cls) | ||
| if not ( | ||
| isinstance(inferred, nodes.ClassDef) | ||
| and inferred.qname() in MATCH_CLASS_SELF_NAMES | ||
| ): | ||
| self.add_message( | ||
| "match-class-positional-attributes", | ||
| node=node, | ||
| confidence=HIGH, | ||
| ) |
There was a problem hiding this comment.
Looking at the sentry results, I do wonder if this is to strict and we'd end up emitting too many false positives. self matches also work for every subclass of the special builtin classes (as long as they don't implement __match_args__). Most notably NamedTuple.
There was a problem hiding this comment.
Added some special casing for tuple that should handle it.
This comment has been minimized.
This comment has been minimized.
cdce8p
force-pushed
the
additional-match-checks
branch
from
3ff329c to
7c078e5
Compare
cdce8p
force-pushed
the
additional-match-checks
branch
from
7c078e5 to
768c4c9
Compare
This comment has been minimized.
This comment has been minimized.
| "can be avoided.", | ||
| ), | ||
| "R1906": ( | ||
| "Use keyword attributes instead of positional ones", |
There was a problem hiding this comment.
| "Use keyword attributes instead of positional ones", | |
| "Use keyword attributes instead of positional ones ('%s')", |
I think we can construct the expected here too. (And one day use it to autofix?)
| case {"type": "user", "data": user_data}: | ||
| match user_data: # Nested match adds complexity | ||
| case {"name": str(name)}: | ||
| case {"name": str() as name}: |
There was a problem hiding this comment.
This check already making pylint better 😄
There was a problem hiding this comment.
Fixed most of the pylint code in #10580 already. That's why there are "only" two instances in a test case left here.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
cdce8p
force-pushed
the
additional-match-checks
branch
from
18c133e to
f420f28
Compare
|
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit f420f28 |
| "attribute in a class pattern.", | ||
| ), | ||
| "R1905": ( | ||
| "Use '%s() as %s' instead", |
There was a problem hiding this comment.
| "Use '%s() as %s' instead", | |
| "Use '%s() as %s' instead, if performance is a concern", |
(This is a suggestion, the message is short, there's room for explanation in it, maybe ?)
There was a problem hiding this comment.
Seems Daniel merged it before I saw your comment 😄
Anyway, not sure we need an extra comment here. I did think about moving these checks to an extension (e.g. CodeStyle. However, there are clear performance benefits which is why I decided to keep them always enabled.
If we get reports about them, we could still consider amending the message or moving the check.
There was a problem hiding this comment.
Oops, sorry! I saw Pierre's approval and thought it was a comment from his earlier review.
Will be more careful next time!
There was a problem hiding this comment.
No problem, it was good to merge which is why I approved in the first place 😄
DanielNoord
temporarily deployed
to
Backport
GitHub Actions
Inactive
Description
Followup to #10559
Add additional checks for match class patterns.
match-class-bind-selfmatch-class-positional-attributes