Make Map subscriptable

[antonagestam]

Fixes #12

[1st1]

Oh right, so should we make the pure Python version also implement class_get_item instead?

Yes, that's the idea.

Because I believe Mapping.register(Map) is not sufficient for making Map[foo, bar] return Map?

True. We only need the Mapping.register() part to make isinstance(immumap, Mapping) work.

[antonagestam]

@1st1 Gotcha, thanks for taking the time to explain! I'll try to update the PR soon :)

[antonagestam]

I added fixes that addresses your comments as well as a test case. Perhaps the documentation should be updated with an example as well?

[1st1]

Looks great! Only a few nits left.

[antonagestam]

I have a few questions on the mutation API.

Should Map.mutate() always return a MapMutation with it's own signature? E.g. Map[A, B]().mutate() => MapMutation[A, B]? I think that would make sense as then MapMutation.finish() would return the same type Map, e.g. Map[A, B]().mutate().finish() => Map[A, B].

As for both MapMutation and Map should I add type hints for all methods or just the ones that are part of the public API? There's a few methods that always raise an exception, like MapMutation.__iter__(), should these have the return type typing.NoReturn or just not be hinted?

I'm also curious as to why MapMutation.pop() accepts a variadic *args and then raises TypeError if it's longer than 1. Why not use a non-variadic single default argument? :)

[1st1]

Should Map.mutate() always return a MapMutation with it's own signature? E.g. MapA, B.mutate() => MapMutation[A, B]?

Yes.

As for both MapMutation and Map should I add type hints for all methods or just the ones that are part of the public API?

Ideally, yes.

There's a few methods that always raise an exception, like MapMutation.iter(), should these have the return type typing.NoReturn or just not be hinted?

I guess using typing.NoReturn makes sense.

I'm also curious as to why MapMutation.pop() accepts a variadic *args and then raises TypeError if it's longer than 1. Why not use a non-variadic single default argument? :)

This is to emulate a "positional-only argument", IIRC.

[antonagestam]

I added more type hints, so the mutation API should now be covered as well as the keys, values and items methods. I added a BitmapNode as an empty class because it is exposed as the type of the m arguments in __init__ methods. However, it's implementation was not easy to decipher and so I left it untyped for now. I don't think I'll be able to figure out the correct type hints for it.

[asvetlov]

I guess mapping views should derive from corresponding collections.abc.

Suggested change

	class MapKeys(Generic[K]):
	class MapKeys(Generic[K], collections.abc.KeysView):

in this case

[antonagestam]

There's a corresponding generic type that we could use: https://docs.python.org/3/library/typing.html#typing.KeysView

Looking at this table in the collections docs it looks like the views also need to implement __contains__ to fulfill the interface?

[asvetlov]

Yes, typing.KeysView is even better.
The definition can be reduced to

class MapKeys(typing.KeysView[K]):
    pass

MapKeys should implement set operations though to conform to the ABC requirement.

[antonagestam]

I agree that it would probably make sense for these classes to conform to those interfaces, but maybe we could leave that to a separate PR? The reason is that I am pretty uncomfortable with writing C. I think the PR in its current scope could make sense as a first step towards a typed interface. What do you think? :)

[asvetlov]

Good to me but @1st1 makes decisions here :D

[1st1]

@ambv

[ambv]

Looks like a good start. Let's merge as is, I'll test this with EdgeDB and current Mypy to see how this behaves. If there's problems, we'll fix them forward.

[antonagestam]

Ping me if you need me to rebase or make other changes :)

[1st1]

Merged.

@antonagestam thank you for your hard work!

@ambv please test this with edgedb codebase. If all good, I'll release a new version.

[ambv]

@1st1, so far, so good. Tested with EdgeDB and Mypy, behaves well.

[1st1]

Alright, I'll schedule a release now.

[1st1]

@1st1, so far, so good. Tested with EdgeDB and Mypy, behaves well.

immutables@0.10 is out, you can now bump the EdgeDB dep.

[jab]

Late to the party for this PR, but spotted a few things and maybe it's better late than never?

(Currently working on type hints for my https://github.com/jab/bidict library, which is why I'm looking around at other Mapping implementations' type hints.)

[jab]

Why "bound=Hashable" here when it does not appear in the KT TypeVar used along with the Mapping supertype itself: https://github.com/python/typeshed/blob/master/stdlib/3/typing.pyi#L60

[antonagestam]

I think my intention here was that having that bound will give the user a type error for non-hashable keys instead of a runtime error.

[jab]

Do the "bound=Any" here buy anything?

[antonagestam]

Just a matter of style, I prefer to more clearly communicate whenever type vars are unbounded.

[jab]

Should the V TypeVar set covariant=True? That would match https://github.com/python/typeshed/blob/master/stdlib/3/typing.pyi#L413 better.

[antonagestam]

Yes, that seems reasonable.