Use match statement in checkers (2) #10528
Conversation
cdce8p
added
Maintenance
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (98.84%) 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 #10528 +/- ##
==========================================
+ Coverage 95.84% 95.88% +0.03%
==========================================
Files 177 177
Lines 19289 19346 +57
==========================================
+ Hits 18488 18549 +61
+ Misses 801 797 -4
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
cdce8p
force-pushed
the
match-explore-2
branch
from
08d0a10 to
b444fff
Compare
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Pierre Sassoulas <[email protected]>
cdce8p
force-pushed
the
match-explore-2
branch
from
b444fff to
933a16c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| continue | ||
| else: | ||
| match inferred := checker_utils.safe_infer(ctx_mgr): | ||
| case _ if not inferred: |
There was a problem hiding this comment.
We're missing the case where isinstance(inferred, util.UninferableBase) that was previously handled ?
There was a problem hiding this comment.
While reviewing the checker code base I've seen the not inferred check in two different ways.
if inferred is None or isinstance(inferred, util.UninferableBase): ...
if not inferred: ...Those could be translated to
match inferred:
case None | util.UninferableBase():
...
case _ if not inferred:
...The later works since bool(util.UninferableBase()) also evaluates to False.
https://github.com/pylint-dev/astroid/blob/v4.0.0b2/astroid/util.py#L42-L43
--
I decided it might be better to just use one of the styles going forward. My slight preference would be towards case _ if not inferred.
| @@ -268,7 +268,7 @@ def new_line(self, tokens: TokenWrapper, line_end: int, line_start: int) -> None | |||
| def process_module(self, node: nodes.Module) -> None: | |||
| pass | |||
|
|
|||
| # pylint: disable-next = too-many-return-statements, too-many-branches | |||
| # pylint: disable-next = too-many-return-statements | |||
There was a problem hiding this comment.
This might actually be a bug. I'd bet the checker doesn't account for match-case.
Update
| case nodes.Assign( | ||
| targets=[nodes.AssignName(), *_], value=nodes.Lambda() as value |
There was a problem hiding this comment.
node is always going to be a nodes.Assign, shouldn't we match node.targets[0] ?
There was a problem hiding this comment.
We also need to match node.value. Technically this introduces one redundant isinstance check however I think the readability improvement makes up for that.
I've also read somewhere that they wanted to cache the isinstance check results but haven't checked if the CPython type actually implemented it.
--
In theory we could also replace nodes.Assign(...) with just object(...). That would work two though I don't think it would be particularly better.
| msg = "non-ascii-name" | ||
|
|
||
| # Some node types have customized messages | ||
| if node_type == "file": | ||
| msg = "non-ascii-file-name" | ||
| elif node_type == "module": | ||
| msg = "non-ascii-module-import" | ||
| match node_type: | ||
| case "file": | ||
| msg = "non-ascii-file-name" | ||
| case "module": | ||
| msg = "non-ascii-module-import" | ||
| case _: | ||
| msg = "non-ascii-name" |
| if ( | ||
| isinstance(func, astroid.BoundMethod) | ||
| and isinstance(func.bound, astroid.Instance) | ||
| and func.bound.name in {"str", "unicode", "bytes"} | ||
| ): | ||
| if func.name in {"strip", "lstrip", "rstrip"} and node.args: | ||
| arg = utils.safe_infer(node.args[0]) | ||
| if not isinstance(arg, nodes.Const) or not isinstance(arg.value, str): | ||
| return | ||
| if len(arg.value) != len(set(arg.value)): | ||
| self.add_message( | ||
| "bad-str-strip-call", | ||
| node=node, | ||
| args=(func.bound.name, func.name), | ||
| ) | ||
| elif func.name == "format": | ||
| self._check_new_format(node, func) | ||
| match func := utils.safe_infer(node.func): | ||
| case astroid.BoundMethod( | ||
| bound=astroid.Instance(name="str" | "unicode" | "bytes" as bound_name), | ||
| ): |
There was a problem hiding this comment.
The match case really shine in those cases.
There was a problem hiding this comment.
I completely agree. There will be more, many more 😉 (#10531)
| if isinstance(default1, nodes.Const) and isinstance(default2, nodes.Const): | ||
| if default1.value != default2.value: | ||
| return True | ||
| elif isinstance(default1, nodes.Name) and isinstance(default2, nodes.Name): | ||
| if default1.name != default2.name: | ||
| match (default1, default2): | ||
| case [nodes.Const(), nodes.Const()]: | ||
| return default1.value != default2.value # type: ignore[no-any-return] | ||
| case [nodes.Name(), nodes.Name()]: | ||
| return default1.name != default2.name # type: ignore[no-any-return] | ||
| case _: |
There was a problem hiding this comment.
A lot better here too !
| if ( | ||
| ( | ||
| isinstance(parent_node, nodes.AnnAssign) | ||
| and parent_node.annotation == current_node | ||
| ) | ||
| or ( | ||
| isinstance(parent_node, nodes.Arguments) | ||
| and current_node | ||
| in ( | ||
| *parent_node.annotations, | ||
| *parent_node.posonlyargs_annotations, | ||
| *parent_node.kwonlyargs_annotations, | ||
| parent_node.varargannotation, | ||
| parent_node.kwargannotation, | ||
| ) | ||
| ) | ||
| or ( | ||
| isinstance(parent_node, nodes.FunctionDef) | ||
| and parent_node.returns == current_node | ||
| ) | ||
| ): | ||
| return True | ||
| match parent_node: | ||
| case nodes.AnnAssign(annotation=ann) if ann == current_node: | ||
| return True | ||
| case nodes.Arguments() if current_node in ( | ||
| *parent_node.annotations, | ||
| *parent_node.posonlyargs_annotations, | ||
| *parent_node.kwonlyargs_annotations, | ||
| parent_node.varargannotation, | ||
| parent_node.kwargannotation, | ||
| ): | ||
| return True | ||
| case nodes.FunctionDef(returns=ret) if ret == current_node: | ||
| return True |
There was a problem hiding this comment.
At some point we need a competition to find the best match uses 😄
|
|
||
|
|
||
| def has_starred_node_recursive( | ||
| node: nodes.For | nodes.Comprehension | nodes.Set, | ||
| node: nodes.For | nodes.Comprehension | nodes.Set | nodes.Starred, |
|
Think I've address all your comments. Let me know if I missed anything.
I've I did my job right, and I'm pretty confident in these, there shouldn't be any behavior changes. At most one or two minor regressions which could be fixed easily. Especially with the improved readability of the match statements. -- |
This comment has been minimized.
This comment has been minimized.
To review I'd suggest to use
Hide whitespace.