graphql-relay-py icon indicating copy to clipboard operation
graphql-relay-py copied to clipboard

Update typing

Open markedwards opened this issue 2 years ago • 6 comments

The release of mypy 1.0 revealed some issues in the typing that I helped add previously:

  1. The PageInfoType protocol is missing some @property decorators
  2. The typing on the *Constructor protocols is overly restrictive. It requires the return values to be of specific shapes, but actually these constructors should not be this strict. For instance, one use case for the PageInfoConstructor is creating a class which uses snake_case instead of camelCase for the properties. The implementation of graphql-relay-py library is not opinionated about this, so the typing should not be either.
  3. The connection_from_array and connection_from_array_slice might not return a ConnectionType, due to 2 above. So @overload variants are added here to more accurately match the behavior.

I've also added a Traversable protocol, which specifies and object with a cursor property. This is the minimum requirement for the return of EdgeConstructor, since the code will crash without a cursor.

In addition, updating the Github workflows and black formatting here.

I spent some time attempting to make the typing much more accurate using generic parameters on the various protocols, but parameterized protocols don't work properly when applied to arguments at the moment. Its also currently not possible with mypy to set a default value on an argument with a type variable due to this issue, which makes the number of overloads explode. Hopefully these problems will be resolved and the typing on graphql-relay-py can be further improved.

markedwards avatar Feb 27 '23 18:02 markedwards

I'm not sure what should be done about the lint failure here. There's a manifest check that's failing. Maybe setup.py should be deleted from the repo?

markedwards avatar Feb 27 '23 18:02 markedwards

Thanks @markedwards, will look into this. Yes, the setup.py should go, I already removed it in GraphQL-core and planned to revisit GraphQL-relay after the next release anyway.

Cito avatar Feb 27 '23 19:02 Cito

It is possible some or all of the *Type protocols are not useful and should be deprecated or removed. I think this decision should perhaps wait until a fully-typed version of the connection functions is possible though. Ideally, the typing on those should enforce correct usage and right now it’s possible to provide conflicting constructors. Also, the Any return types are silly, but we need to wait for mypy to fix bugs. :-/

markedwards avatar Feb 27 '23 19:02 markedwards

Also, probably a good idea to remove 3.6 support and use the latest mypy? And also, add 3.11 testing and support?

I didn't want to do these things in this PR because they are a separate consideration and also I don't fully understand the poetry/tox setup.

markedwards avatar Feb 28 '23 09:02 markedwards

Sure. This is already changed in GraphQL-core and will of course also change in GraphQL-relay.

Cito avatar Feb 28 '23 09:02 Cito

In terms of further improving the typing here, I have to say it's not quite clear to me what the forward path is yet. Clearly this bug is blocking, because connection_type, edge_type and page_info_type need to be generic.

I think what might work in the end is constraining edge_type and page_info_type to the Edge and PageInfo named tuple classes, in the case that connection_type is not passed. And in the case that it is passed, enforcing return type consistency between connection_type, edge_type, and page_info_type. So, this will be a bunch of @overload with a permissive implementation (probably will end up as a bunch of Any).

I don't think the other issue I mentioned is relevant actually. I think its highly unlikely passing default values for a parameterized argument will ever be implemented (that issue is 5 years old), and its also not clear that it would resolve the complex relationship between connection_type, edge_type, and page_info_type in this case anyway.

So I guess I'll take another stab at it when the blocker is resolved.

markedwards avatar Feb 28 '23 10:02 markedwards