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 variable name to "Need type annotation for variable" error #4496

Merged
merged 5 commits into from Jan 21, 2018

Conversation

Projects
None yet
2 participants
@elliott-beach
Contributor

elliott-beach commented Jan 21, 2018

fixes #4431 by changing "need annotation for variable" messages to "need to annotation for 'x'".

elliott-beach added some commits Jan 21, 2018

@ilevkivskyi

Thanks! This looks straightforward. I just have few comments. Could you please also fix the lint failure (and never use tabs)?

@@ -62,7 +62,7 @@
MUST_HAVE_NONE_RETURN_TYPE = 'The return type of "{}" must be None'
INVALID_TUPLE_INDEX_TYPE = 'Invalid tuple index type'
TUPLE_INDEX_OUT_OF_RANGE = 'Tuple index out of range'
NEED_ANNOTATION_FOR_VAR = 'Need type annotation for variable'
NEED_ANNOTATION_FOR_VAR = 'Need type annotation for variable \"{}\"'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jan 21, 2018

Collaborator

This is now never used, IIUC and should be removed.

@@ -962,6 +962,9 @@ def unimported_type_becomes_any(self, prefix: str, typ: Type, ctx: Context) -> N
self.fail("{} becomes {} due to an unfollowed import".format(prefix, self.format(typ)),
ctx)
def need_annotation_for_var(self, node: SymbolNode, context: Context) -> None:
self.fail("Need type annotation for \'{}\'".format(node.name()), context)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jan 21, 2018

Collaborator

You don't need to escape quotes here.

==
main:5: error: Need type annotation for variable
main:5: error: Need type annotation for 'x'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jan 21, 2018

Collaborator

I think you should also add dedicated tests for more tricky situations like:

x, y = [], []
self.a, self.b = None, None

(and maybe some others if you have more ideas).

This comment has been minimized.

@elliott-beach

elliott-beach Jan 21, 2018

Contributor

I added a case for multiple assignment. I think classes would be a valid case too but currently mypy doesn't appear to require annotations for self assignments. Other cases I can't think of. My understanding is the only assignments that can trigger this are assignments with variable names literal as lvalue, which is covered.

elliott-beach added some commits Jan 21, 2018

@elliott-beach

This comment has been minimized.

Contributor

elliott-beach commented Jan 21, 2018

Sorry, I will lint before pushing next time. I had an interesting configuration issue with PyCharm, causing the tabs, which I just resolved.

@@ -1787,3 +1787,8 @@ reveal_type(a.__pow__(2)) # E: Revealed type is 'builtins.int'
reveal_type(a.__pow__(a)) # E: Revealed type is 'Any'
a.__pow__() # E: Too few arguments for "__pow__" of "int"
[builtins fixtures/ops.pyi]
[case testTypeAnnotationNeededMultipleAssignment]
x,y = [],[] # E: Need type annotation for 'x' \

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jan 21, 2018

Collaborator

I know this is just a test but why not use PEP 8 rules here? (space after comma)

This comment has been minimized.

@elliott-beach

elliott-beach Jan 21, 2018

Contributor

yes yes

This comment has been minimized.

@elliott-beach

elliott-beach Jan 21, 2018

Contributor

because I need to read PEP 8 :P

@ilevkivskyi

LGTM now.

@ilevkivskyi ilevkivskyi merged commit 86f4e01 into python:master Jan 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment