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

Treat any instance of a Mapping as a Dict. #45

Closed
wants to merge 2 commits into from

Conversation

rowillia
Copy link
Contributor

Without this we're winding up with a bunch types like:

Union[Dict[str, str], OrderedDict]

On functions that can accept any Mapping.

I was woried about covariance/contravariance with this and mypy seems to be OK with that:

from collections import OrderedDict
from typing import Dict

def foo(a: Dict[str, str]) -> Dict[str, str]:
    return OrderedDict([('a', 'a'), ('b', 'b')])

foo(OrderedDict([('a', 'a'), ('b', 'b')]))

Without this we're winding up with a bunch types like:

```
Union[Dict[str, str], OrderedDict]
```

On functions that can accept any `Mapping`.

I was woried about covariance/contravariance with this and `mypy` seems to be OK with that:

```python
from collections import OrderedDict
from typing import Dict

def foo(a: Dict[str, str]) -> Dict[str, str]:
    return OrderedDict([('a', 'a'), ('b', 'b')])

foo(OrderedDict([('a', 'a'), ('b', 'b')]))
```
@gvanrossum
Copy link
Contributor

Can you change this translation to the second pass (i.e. pyannotate_tools/annotations)? The isinstance(..., collections.Mapping) check worries me (it could be slow, and there was a bug here where typing_extensions caused endless recursion in such a check -- #36).

@rowillia
Copy link
Contributor Author

@gvanrossum Thanks for the feedback! Instead of using isinstance I'm specifically checking for the most common ones I found (defaultdict and OrderedDict) now. There's a test case here to verify there's no infinite recursion, anything else I could/should test?

@gvanrossum
Copy link
Contributor

I'm still not happy with doing any kind of unification of this kind during runtime collection. There are important API differences between Dict and OrderedDict and I don't want those to be lost in type_info.json. You haven't explained to me why you can't do this in the second pass.

@rowillia
Copy link
Contributor Author

@gvanrossum The reason this is happening in the first pass is if we do this in the second pass we don't collect any information on key/value types. We can add a DefaultDict and OrderedDict type to type collection as well and then unify those in the second pass if you would prefer.

@gvanrossum
Copy link
Contributor

Oh! But I would like to still keep the actual type (dict, collections.defaultdict or collections.OrderedDict) in type_info.json. If you can do that I think this is a great improvement!

@gvanrossum
Copy link
Contributor

Do you think you will have time for this soon? I am waiting to release version 1.0.3.

@rowillia
Copy link
Contributor Author

@gvanrossum Working on it now!

@gvanrossum
Copy link
Contributor

@rowillia Are you still interested in pursuing this? I need to decide whether to release PyAnnotate 1.0.3 with or without it. It will likely be the last release this year due to my vacation schedule and I'd like to release it today or Monday 12/11 at the latest.

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.

None yet

2 participants