-
Notifications
You must be signed in to change notification settings - Fork 41
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
base: main
Are you sure you want to change the base?
Update typing #48
Conversation
I'm not sure what should be done about the lint failure here. There's a manifest check that's failing. Maybe |
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. |
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 |
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. |
Sure. This is already changed in GraphQL-core and will of course also change in GraphQL-relay. |
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 I think what might work in the end is constraining 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 So I guess I'll take another stab at it when the blocker is resolved. |
The release of mypy 1.0 revealed some issues in the typing that I helped add previously:
PageInfoType
protocol is missing some@property
decoratorsPageInfoConstructor
is creating a class which uses snake_case instead of camelCase for the properties. The implementation ofgraphql-relay-py
library is not opinionated about this, so the typing should not be either.connection_from_array
andconnection_from_array_slice
might not return aConnectionType
, 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 acursor
property. This is the minimum requirement for the return ofEdgeConstructor
, since the code will crash without acursor
.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.