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

Add arg_names attribute to FunctionContext and MethodContext #5918

Merged
merged 4 commits into from Dec 16, 2018

Conversation

Projects
None yet
2 participants
@mkurnikov
Copy link
Contributor

mkurnikov commented Nov 19, 2018

References #5409.

@mkurnikov mkurnikov changed the title Add arg_names attribute to FunctionContext and MethodContext [WIP] Add arg_names attribute to FunctionContext and MethodContext Nov 19, 2018

@mkurnikov mkurnikov changed the title [WIP] Add arg_names attribute to FunctionContext and MethodContext Add arg_names attribute to FunctionContext and MethodContext Nov 20, 2018

@mkurnikov mkurnikov changed the title Add arg_names attribute to FunctionContext and MethodContext [WIP] Add arg_names attribute to FunctionContext and MethodContext Nov 26, 2018

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

TBH this PR looks a bit premature, and heavily over-engineered. I added a comment in the issue #5409 (comment) about how I think this should be done. You can probably keep the extensive tests, but I would propose to reimplement this as I suggested (unless I am missing something this will be just few lines of code).

Thanks for working on this!

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Dec 6, 2018

@mkurnikov Are you planning to work on this soon? Absolutely no pressure, but I just wanted to let you know I will be glad to use the new context attributes as well.

@mkurnikov

This comment has been minimized.

Copy link
Contributor Author

mkurnikov commented Dec 7, 2018

I'll finish it today or on the weekend. With the recent refactoring, it will be even easier.

@mkurnikov mkurnikov force-pushed the mkurnikov:add-arg-names-to-function-method-contexts branch from 130d57c to 8b559bb Dec 8, 2018

@mkurnikov

This comment has been minimized.

Copy link
Contributor Author

mkurnikov commented Dec 8, 2018

WIP, missing tests for arg_kinds, existing tests should be rewritten and docs for new context attributes. Will try to finish on this weekend.

Explanation of callee_arg_names:
Guido mentioned in the
#5409 (comment)

I needed the names of the arguments but they were not directly available. I ended up using rtype.type.names['__init__'].node.arg_names (with various type and None guards) and subtracting 1 to compensate for self, which looked very hacky. It would be nice if FunctionContext also has an arg_names argument.

I've encountered the same thing in django-stubs.

Five arrays of the same length don't look great, but I couldn't find any other way.

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks for the update! This looks very good, basically just the things you mentioned in the last comment are missing, this can be merged after you add them. Here are also few suggestions.

Show resolved Hide resolved mypy/plugin.py
Show resolved Hide resolved mypy/plugin.py
@@ -230,6 +230,157 @@ def decorator2() -> Callable[..., Callable[..., int]]: pass
[[mypy]
plugins=<ROOT>/test-data/unit/plugins/named_callable.py

[case testFunctionMethodContextsHasArgNames]
# flags: --config-file tmp/mypy.ini
from mod import Klass, func

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 8, 2018

Collaborator

This is very personal, but I don't like Klass, I would really prefer just Class.

Show resolved Hide resolved test-data/unit/check-custom-plugin.test
Show resolved Hide resolved mypy/plugin.py
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Dec 13, 2018

@mkurnikov Sorry for another ping, will you have time yo finish this PR any time soon? (This blocks some other things.)

@mkurnikov

This comment has been minimized.

Copy link
Contributor Author

mkurnikov commented Dec 13, 2018

I'll finish it tomorrow.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Dec 13, 2018

Great, thanks!

@mkurnikov mkurnikov force-pushed the mkurnikov:add-arg-names-to-function-method-contexts branch from 8b559bb to f98d920 Dec 14, 2018

@mkurnikov mkurnikov changed the title [WIP] Add arg_names attribute to FunctionContext and MethodContext Add arg_names attribute to FunctionContext and MethodContext Dec 14, 2018

ilevkivskyi added some commits Dec 16, 2018

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

OK, I will now add few more minor fixes. It will be easier for me to just push here instead one more round of review.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Dec 16, 2018

Also please never rebase, it is much harder to review, and GitHub only sends a notification when you push a new commit.

@ilevkivskyi ilevkivskyi merged commit ed248c8 into python:master Dec 16, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Dec 16, 2018

Thanks for the PR!

@mkurnikov mkurnikov deleted the mkurnikov:add-arg-names-to-function-method-contexts branch Dec 16, 2018

@mkurnikov

This comment has been minimized.

Copy link
Contributor Author

mkurnikov commented Dec 16, 2018

Got it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment