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

Update typing #48

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

markedwards
Copy link
Contributor

@markedwards markedwards commented Feb 27, 2023

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
Copy link
Contributor Author

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?

@Cito
Copy link
Member

Cito commented Feb 27, 2023

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.

@markedwards
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@Cito
Copy link
Member

Cito commented Feb 28, 2023

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

@markedwards
Copy link
Contributor Author

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.

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.

2 participants