Bug pylint 2588 #632
Bug pylint 2588 #632
Conversation
…ing deleting statement in _filter_statements method if the node's name is the same as the current statement's name
…er_stmts method of the LookupMixin class in the case where the node is a PartialFunction and its name if the same as tue current statement's name
…test that ensures that after using partial on a function, a subsequent call of this function is still well inferred.
|
@PCManticore it seems that Travis is failing due to formatting standard. |
astroid/tests/unittest_brain.py
Outdated
|
|
||
|
|
||
| class TestFunctoolsPartial: | ||
| class TestFunctoolsPartial(unittest.TestCase): |
There was a problem hiding this comment.
@hippo91 It's not required to add unittest.TestCase here because we use pytest and it is able to find these tests accordingly.
|
@hippo91 I think it could work locally with Python 3.6 if you use pyenv to manage your Python installations. It should be a matter of Regarding the thing with |
astroid/node_classes.py
Outdated
| if not (optional_assign or are_exclusive(_stmts[pindex], node)): | ||
| del _stmt_parents[pindex] | ||
| del _stmts[pindex] | ||
| if not hasattr(node, 'is_partial_function') or node.name != _stmts[pindex].name: |
There was a problem hiding this comment.
What's the reason for the second check, can you add a comment for that?
astroid/brain/brain_functools.py
Outdated
|
|
||
| filled_positionals = len(call.positional_arguments[1:]) | ||
| filled_keywords = list(call.keyword_arguments) | ||
| is_partial_function = True |
There was a problem hiding this comment.
@hippo91 I wonder if we can extract PartialFunction somehow from this module. We can use astroid.objects for it, and if we need to figure out if a function is partial or not, we can implement the name property and qname function, just like we do for Super, FrozenSet and friends. And instead of checking if is_partial_function is given, we only look for .name against a predefined list of elements for which to apply the condition in the mixin. Do you think that's worth it?
There was a problem hiding this comment.
@PCManticore it looks interesting.
I work on it.
…ng it into objects. Implementing its qname as the class's name. Testing this qname in LookupMixin._filter_stmts (node_classes)
|
@PCManticore thank you for the hint concerning pyenv. It's perfect. Now i can use I still can't have CI ok, because of |
There was a problem hiding this comment.
Looks great! Regarding black, are you using black==18.6b4 or tox -e formatting? That should be enough to make black happy here. Also feel free to ignore the new linting errors, they usually occur when we update pylint with a new check, since we use the master version in `tox.
|
@PCManticore I succeeded in correcting the formatting error by using the same version of black. Thank you. |
|
Oh gotcha, I see what you mean now. The CI jobs are now grouped into stages, so first we run anything linting / formatting related, then followed by the actual tests. In this case, I think it's best to fix the linting errors in a separate PR, merge it and then rebase this one to include those fixes. also I'll be away for two weeks so won't have time to fix those myself. :) |
|
Hey @hippo91 I merged this manually and pushed it to |
2 participants
Description
This PR fixes the pylint-dev/pylint#2588. The problem was that a call to a function that has been
previously called via
functools.partialwas wrongly inferred.The bug comes from the
_filter_stmtsmethod of theLookupMixinclass. The deletion of the current statement should not be made in the case where the node is an instance of thePartialFunctionclass(
brain_functools) and if the node's name is the same as the statement's name.I tried to use
isinstancefunction to test if the node is an instance of thePartialFunction. The problem is that this class is locally declared inside the_functools_partial_inferencefunction ofbrain_functoolsmodule. Of course i was thinking of extracting this class in its own module but as it inherits fromFunctionDefthis module should includescoped_nodeswhich itself includesnode_classes. And i need to include this new module intonode_classes. I'am facing a circular import problem. Thus i preferred to make as less changes as possible and created a class attributeis_partial_functionand test the presence of this attribute viahasattrto determine if the class is an instance ofPartialFunction.I added a test in the
test_inferred_partial_function_callsof theTestFunctoolsPartialclass.Apparently this class was not tested during the CI because it doesn't inherit from
unittest.TestCase!I changed this but i think it will be needed also for other classes of the
unittest_brainmodule.@PCManticore am i wrong?
Type of Changes
Related Issue
Closes pylint-dev/pylint#2588