Fix bug with cell-var-from-loop and kw_defaults #5045
Merged
Conversation
This file contains 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
Pierre-Sassoulas
requested changes
Sep 20, 2021
pylint/checkers/utils.py
Outdated
| @@ -402,7 +402,10 @@ def is_default_argument( | |||
| if not scope: | |||
| scope = node.scope() | |||
| if isinstance(scope, (nodes.FunctionDef, nodes.Lambda)): | |||
| for default_node in scope.args.defaults: | |||
| all_defaults = scope.args.defaults + [ | |||
| d for d in scope.args.kw_defaults if d is not None | |||
There was a problem hiding this comment.
We're calculating the full list of kwargs here (even if the first element of the list would return True) so we could improve performance by using a generator line 408 in for default_node in all_defaults:.
pylint/checkers/utils.py
Outdated
| @@ -402,7 +402,10 @@ def is_default_argument( | |||
| if not scope: | |||
| scope = node.scope() | |||
| if isinstance(scope, (nodes.FunctionDef, nodes.Lambda)): | |||
| for default_node in scope.args.defaults: | |||
| all_defaults = scope.args.defaults + [ | |||
There was a problem hiding this comment.
return any(
default_name_node is node
for default_name_node in default_node.nodes_of_class(nodes.Name)
for default_node in scope.args.defaults + d for d in scope.args.kw_defaults if d is not None
)Maybe something like this would work ? I'm a little rusty with the imbricated for loop.
There was a problem hiding this comment.
Nice catch, yeah I was being lazy before. You can't directly concatenate a list and a generator, but I used itertools.chain to the same effect.
|
@Pierre-Sassoulas the "Process test coverage" CI test failed, but I don't think that's because of my changes? |
Pierre-Sassoulas
approved these changes
Sep 20, 2021
| for default_name_node in default_node.nodes_of_class(nodes.Name): | ||
| if default_name_node is node: | ||
| return True | ||
| all_defaults = itertools.chain( |
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.
Type of Changes
Description
The fix in #4827 made use of
pylint.checkers.utils'sis_default_argumentfunction, which currently doesn't handle default values for keyword-only parameters:https://github.com/PyCQA/pylint/blob/314cb9ffd369b5ab99688c885eb537c901e861eb/pylint/checkers/utils.py#L396-L409
Default values for keyword-only parameters are stored in a separate
args.kw_defaultsattribute.This led to #5012. I fixed this by modifying the function to check
kw_defaultsas well. Note that since keyword-only parameters can have no default argument, we need to filter outNonevalues from this attribute to keep only the astroid nodes representing the actual defaults.Closes #5012.