Skip to content

Better signature for Mapping.get(). #552

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

Closed
wants to merge 1 commit into from
Closed

Better signature for Mapping.get(). #552

wants to merge 1 commit into from

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Sep 16, 2016

Fixes #278.

The approach here is different from what we thought up before. According
to this commit, there are three valid forms of Mapping[K, V].get():

  d.get(k)        # returns Optional[V]
  d.get(k, None)  # also returns Optional[V]  (seen in the wild)
  d.get(k, v)     # requires v to have type V, and returns V

So we don't allow d.get(k, d) where d is a type unrelated to V. This is
a compromise, but I don't think there's much use for d.get(d) with a
default that is differently-typed than V, and when I tried it my
application code (that didn't use this) wouldn't type-check correctly.

Fixes #278.

The approach here is different from what we thought before.  According
to this commit, there are three valid forms of Mapping[K, V].get():

  d.get()      # returns Optional[V]
  d.get(None)  # also returns Optional[V]  (seen in the wild)
  d.get(v)     # requires v to have type V, and returns V

So we don't allow d.get(d) where d is a type unrelated to V.  This is
a compromise, but I don't think there's much use for d.get(d) with a
default that is differently-typed than V, and when I tried it my
application code (that didn't use this) wouldn't type-check correctly.
@gvanrossum
Copy link
Member Author

@JukkaL @matthiaskramm Any thoughts on this interminable issue?

Should we also support the signature (K, Optional[V]) -> Optional[V]?

@matthiaskramm
Copy link
Contributor

Seems fine to me.

Isn't (K, Optional[V]) -> Optional[V] already covered by the other cases?

@JukkaL
Copy link
Contributor

JukkaL commented Sep 23, 2016

The overload doesn't work well with mypy, since mypy doesn't support @overloads outside stub files. If a user defines a subclass of Mapping, they can't implement get with the correct signature.

@gvanrossum
Copy link
Member Author

The overload doesn't work well with mypy, since mypy doesn't support @Overloads outside stub files.

We should fix the latter. But until then, what do you suggest?

@JukkaL
Copy link
Contributor

JukkaL commented Sep 23, 2016

I don't know really. Overloading seems to be the only way to give get() a precise signature, except maybe for the following alternative (if we stretch the semantics a a bit):

_DT = TypeVar('_DT')

class Mapping(...):
    def get(self, key: _KT, default: _DT = None) -> Union[_VT, _DT]: ...

This won't work either with current mypy, but this is arguably a simpler signature and might be easier to support. Here's how we might make this might work:

  • _DT is only used for the default value of get.
  • d.get(key) should result in inferring None as the value of _DT, since the default isn't given an explicit value in the call.
  • d.get(key, '') should infer str, not Optional[str] as the value of _DT, since the default value None is not used.

It would be good if mypy would support one of these alternatives properly (@overload or my alternative proposal) as otherwise we are likely to break user code. So I wonder if this can wait a bit? Another idea is to somehow have a different get signature in mypy's strict optional mode.

@gvanrossum
Copy link
Member Author

OK, I'll just close this PR until we have the mypy side of things sorted out.

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.

3 participants