Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if decorator returns use keyword (unexpected-keyword-arg) #5547

Merged
merged 5 commits into from
Dec 18, 2021

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature, or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Type of Changes

Type
🐛 Bug fix

Description

Closes #258 (🎉)

I'd like to see the coverage report on this. I might need to add some additional tests.

@DanielNoord DanielNoord added Decorators False Positive 🦟 A message is emitted but nothing is wrong with the code labels Dec 17, 2021
@DanielNoord DanielNoord added this to the 2.13.0 milestone Dec 17, 2021
@coveralls
Copy link

coveralls commented Dec 17, 2021

Pull Request Test Coverage Report for Build 1594454584

  • 21 of 21 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 93.689%

Totals Coverage Status
Change from base Build 1594420876: 0.009%
Covered Lines: 14295
Relevant Lines: 15258

💛 - Coveralls

@DanielNoord
Copy link
Collaborator Author

Blocked by #5549. Rest of the tests and coverage look good so this is ready for review.

@DanielNoord DanielNoord marked this pull request as ready for review December 17, 2021 20:40
@DanielNoord DanielNoord added the Blocked 🚧 Blocked by a particular issue label Dec 17, 2021
@Pierre-Sassoulas Pierre-Sassoulas self-requested a review December 17, 2021 21:57
@DanielNoord DanielNoord removed the Blocked 🚧 Blocked by a particular issue label Dec 17, 2021
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's heart-warming to see really old issues getting addressed 🎉

@@ -0,0 +1,118 @@
"""Tests for unexpected-keyword-arg"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are on point !


# If we can't infer the decorator we assume it satisfies consumes
# the keyword, so we don't raise false positives
if not inferred:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really related to this MR but if we have to handle confidence level, this kind of return where we do not exactly know if inference was used or not in the calling code will be a headache later. Makes me thing that implementing confidence properly will be an enormous work, maybe not even worth it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thankfully we have UNDEFINED 😄

@Pierre-Sassoulas Pierre-Sassoulas merged commit bfeca4c into pylint-dev:main Dec 18, 2021
@DanielNoord DanielNoord deleted the keyword-decorator branch December 18, 2021 11:09
@martimlobao
Copy link

martimlobao commented Jan 14, 2022

@DanielNoord maybe I should open a new ticket, but just tried this out with the latest version of the main branch and there seems to be a case where this is still raising a false positive unexpected-keyword-arg error. Here's a MWE:

def example_decorator(*new_func_args, **new_func_kwargs):
    def decorator(func):
        def wrapper(*args, **kwargs):
            print_me = kwargs.pop("print_me", None)
            def inner(*inner_args, **inner_kwargs):
                print(print_me)
                return func(*inner_args, **inner_kwargs)

            return inner(*args, **kwargs)

        return wrapper

    if len(new_func_args) == 1 and len(new_func_kwargs) == 0 and callable(new_func_args[0]):
        return decorator(new_func_args[0])
    return decorator

@example_decorator
def example_func(num):
    return num ** 2

example_func(6, print_me="hello")

(Note that no error is raised by pylint if we remove the top-level function (example_decorator). The reason for someone to define the decorator this way is so that it's possible to use the bare @example_decorator, as well as with arguments @example_decorator(foo=1).)

Using pylint on commit hash e75e37a, this is the error that's printed out:

E1123: Unexpected keyword argument 'print_me' in function call (unexpected-keyword-arg)

Calling example_func works fine:

>>> example_task(6, print_me="hello")
hello
36

@DanielNoord
Copy link
Collaborator Author

Ah yes, this won't be catched. Not sure how easy it would be to implement looking for inner return values.
In any case, I think this warrants its own issue. Could you open it?

@martimlobao
Copy link

Ah yes, this won't be catched. Not sure how easy it would be to implement looking for inner return values. In any case, I think this warrants its own issue. Could you open it?

Sorry for taking forever, but finally opened the ticket :)

@DanielNoord
Copy link
Collaborator Author

Thanks @martimlobao! Totally forgot about this myself.

@martimlobao
Copy link

Thanks @martimlobao! Totally forgot about this myself.

lol sorry for the ridiculously incomplete title, i started writing it and then forgot to finish 🤦‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Decorators False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive unexpected-keyword-arg (E1123) for decorators
5 participants