Skip to content

Allow any subtype of typing.Mapping[str, Any] to be passed in as kwargs. #3604

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

Merged
merged 4 commits into from
Jun 27, 2017

Conversation

rowillia
Copy link
Contributor

This fixes #3595

@@ -280,12 +280,22 @@ reveal_type(formatter.__call__) # E: Revealed type is 'def (message: builtins.s
[builtins fixtures/bool.pyi]
[out]

[case testPassingMappingForKeywordVarArg]
from typing import Mapping
def f( **kwargs: 'A') -> None: pass
Copy link
Member

Choose a reason for hiding this comment

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

nit: no space before **

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Looks good!

Just two small comments.

mypy/messages.py Outdated
if isinstance(typ, Instance):
suffix = ', not {}'.format(typ.type.fullname())
self.fail(
'Argument after ** must be a subtype of Mapping{}'.format(suffix),
Copy link
Member

Choose a reason for hiding this comment

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

I would just say must be a mapping (with small m), not must be a subtype of Mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

f(**m)
class A: pass
[builtins fixtures/dict.pyi]

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a test with a custom subclass of Mapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@ilinum ilinum left a comment

Choose a reason for hiding this comment

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

Looks good overall!

Just a couple of small things.

mypy/messages.py Outdated
context)
suffix = ''
if isinstance(typ, Instance):
suffix = ', not {}'.format(typ.type.fullname())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure calling typ.type.fullname() is the best option.
In order to be consistent with other error messages, I would use self.format(typ) instead of typ.type.fullname().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

class dict(Generic[T, S]): pass

class dict(Iterable[T], Mapping[T, S], Generic[T, S]):
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: it seems like all other fixtures have pass on the same line as the class declaration. I would try to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

mypy/messages.py Outdated
context)
suffix = ''
if isinstance(typ, Instance):
suffix = ', not {}'.format(typ)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, what I meant was:
', not {}'.format(self.format(typ))

This would make the user message more user friendly:
Argument after ** must be a mapping, not A instead of Argument after ** must be a mapping, not __main__.A

Copy link
Collaborator

@ilinum ilinum left a comment

Choose a reason for hiding this comment

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

LGTM!

Let's merge the PR when @ilevkivskyi approves it.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Thanks!

@ilevkivskyi ilevkivskyi merged commit a386107 into python:master Jun 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mypy requires argument after ** to be a dict when a Mapping is sufficient
4 participants