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

Refactor typings #62

Merged
merged 2 commits into from Aug 4, 2021
Merged

Conversation

bryanforbes
Copy link
Contributor

  • Improve typing of __init__()
  • Update typing of Map-producing functions to produce the correct type
  • Update typing of other methods to more closely align with Mapping
  • Add protocol classes for unexposed data structures
  • Export protocol classes for ease of use in typed code
  • Update stub file to pass in mypy strict mode

fixes #54

I've taken the work in the typing overhaul PR and improved it as detailed above. The following passes using mypy in strict mode:

from immutables import Map
from typing import Union

m: Map[int, int] = Map()

m1 = Map[int, int]({1: 2})
m2: Map[Union[int, str], int] = m1.update({"answer": 2})
m3: Map[Union[int, str], int] = m1.update(answer=2)
m4: Map[int, Union[int, str]] = m1.update({4: "four"})
m5: Map[Union[int, str], Union[int, str]] = m1.update({"four": "four"})
m6: Map[Union[int, str], Union[int, str]] = m1.update(four="four")
m7: Map[Union[int, str], Union[int, str, float]] = m1.update(m5, five=5.0)


reveal_type(m1.update(m5, four=(4.0,)).update(five=5))  # Revealed type is 'immutables._map.Map[Union[builtins.int, builtins.str], Union[builtins.int, Tuple[builtins.float], builtins.str]]'
reveal_type(m1.set('four', 'four').set((4.0,), (5.0,)))  # Revealed type is 'immutables._map.Map[Union[builtins.int, builtins.str, Tuple[builtins.float]], Union[builtins.int, builtins.str, Tuple[builtins.float]]]'

There was discussion in #54 about restricting update() and other Map-producing functions to the key and value types of the original Map. I'm not exactly sure which is the best approach since the implementation allows update() to take different types for the key and value, however typing.Dict disallows this for dict even though dict.update() allows similar functionality as Map.update(). For now, I've left Map.update() similar to what was done in #54, but that can certainly be changed before merging.

@bryanforbes
Copy link
Contributor Author

I thought about this a bit more and one advantage of disallowing different key and value types is that Map.mutate() returns a MapMutation that will type check correctly and won't need a cast() (or a change to the library code to support syntax like map.mutate[Union[int, str], str]() to use MapMutation.update() with different key and value types.

@elprans
Copy link
Member

elprans commented Feb 10, 2021

I'm strongly in favor of restricting update to original types.

@bryanforbes
Copy link
Contributor Author

I'm strongly in favor of restricting update to original types.

I think I've convinced myself of restriction as well. I'll update the PR.

@bryanforbes
Copy link
Contributor Author

I've updated the PR making update() and other mutators restricted to the Map type. Here's a run-down of what happens now:

from immutables import Map
from typing import Dict, Union, cast

def init() -> None:
    def thing(m: Map[str, Union[str, int]]) -> None:
        ...

    thing(Map(foo=1))
    thing(Map(foo='bar', baz=1))
    thing(Map([('foo', 'bar'), ('bar', 1)]))
    thing(Map(Map(foo=1), bar='foo'))
    m = Map({1: 2})
    thing(m)  # Incompatible type

def assignments() -> None:
    m_int__str = Map[int, str]()
    m_str__str = Map[str, str]()
    m_int_str__str = Map[Union[int, str], str]()
    m_str__int_str = Map[str, Union[int, str]]()

    m_int__str = m_str__str  # Incompatible types
    m_int__str = m_int_str__str  # Incompatible types
    m_int__str = m_str__int_str  # Incompatible types

    m_str__str = m_int__str  # Incompatible types
    m_str__str = m_int_str__str  # Incompatible types
    m_str__str = m_str__int_str  # Incompatible types

    m_int_str__str = m_int__str  # Incompatible types
    m_int_str__str = m_str__str  # Incompatible types
    m_int_str__str = m_str__int_str  # Incompatible types

    m_str__int_str = m_int__str  # Incompatible types
    m_str__int_str = m_int_str__str  # Incompatible types
    m_str__int_str = m_str__str

def update() -> None:
    m_int__str: Map[int, str] = Map()
    m_str__str: Map[str, str] = Map()
    m_int_str__str: Map[Union[int, str], str] = Map()
    m_str__int_str: Map[str, Union[int, str]] = Map()

    m_int__str.update({1: '2'})
    m_int__str.update({1: '2'}, three='4')  # Unexpected keyword argument
    m_int__str.update({1: 2})  # Argument 1 has incompatible type

    m_str__str.update({'1': '2'})
    m_str__str.update({'1': '2'}, three='4')
    m_str__str.update({'1': 2})  # Argument 1 has incompatible type

    m_int_str__str.update(cast(Dict[Union[int, str], str], {1: '2', '3': '4'}))
    m_int_str__str.update({1: '2'}, three='4')
    m_int_str__str.update({'1': 2})  # Argument 1 has incompatible type

    m_str__int_str.update({'1': 2, '2': 3})
    m_str__int_str.update({'1': 2, '2': 3}, four='5')
    m_str__int_str.update({1: 2})  # Argument 1 has incompatible type

def mutate() -> None:
    m = Map[str, str]()

    with m.mutate() as mm:
        mm[0] = '1'  # Invalid index type
        mm['1'] = 0  # Incompatible types
        mm['1'] = '2'
        del mm['1']
        mm.set('3', '4')
        m2 = mm.finish()

    reveal_type(m2)  # Revealed type is 'immutables._map.Map[builtins.str*, builtins.str*]'

@decorator-factory decorator-factory mentioned this pull request Feb 13, 2021
@@ -1,4 +1,4 @@
recursive-include tests *.py
recursive-include immutables *.py *.c *.h *.pyi
include LICENSE README.rst
include immutables/py.typed
include mypy.ini immutables/py.typed
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for adding mypy.ini in the distribution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I did this out of habit. Most of the projects I work on do some sort of type checking during testing. That doesn't seem to be the case for this one.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think it is good idea to add such a test, otherwise the annotations will inevitably rot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elprans would you like me to add a test that runs mypy?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that would work too.

@JanTkacik
Copy link

Hi, can I somehow help to get this to master ?

Copy link
Contributor

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

The typing changes look good!

I also smashed in some tests of the typing by hackily reusing mypy's testing framework. @elprans , you might want to take a look at that part

Copy link
Member

@elprans elprans left a comment

Choose a reason for hiding this comment

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

@msullivan, thanks! Hopefully we'll be able to make mypy more amenable to remove the need for these hacks in the future.

bryanforbes and others added 2 commits August 3, 2021 17:47
* Improve typing of `__init__()`
* Update typing of `Map`-producing functions to produce the correct type
* Update typing of other methods to more closely align with `Mapping`
* Add protocol classes for unexposed data structures
* Export protocol classes for ease of use in typed code
* Update stub file to pass in mypy strict mode
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

6 participants