-
Notifications
You must be signed in to change notification settings - Fork 1
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
[DEV-11572] add: backwards compat static typing #303
base: master
Are you sure you want to change the base?
Conversation
518c533
to
5d24263
Compare
|
||
return raw_response["data"] | ||
|
||
def process_response(self, response: "AnyDict") -> "ResponseType": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typed python does not permit overriding the typing of the signature in subclasses. we were doing that implicitly before by having this fn return Dict
and subclasses return their BaseType
, etc.
a Union[Dict, ResponseType]
does not solve the problem here either, as subclasses would have had to cast
or type: ignore
to deal with the Dict
part in their overrides (ugly, not nice)
the smallest code change soln I could come up with is to split the function into a parse_payload
fn typed to return Any
, and replaced all super().process_response
calls in every query with super().parse_payload
. this impl calls parse_payload
to maintain backwards compat
practically, that means we're deprecating the use of super().process_response
in any custom GraphQLRequest
subclasses; i tried, but sadly could not figure out a nice way to raise a deprecation error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've used https://deprecation.readthedocs.io/en/latest/ in the past for raising deprecation errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, i just couldn't figure out a way to only raise the error when invoking it via super().process_response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my kingdom for working deprecation in python
@@ -28,18 +37,22 @@ def valid_type(v): | |||
) | |||
|
|||
|
|||
# TODO: all the introspection here class makes the typing sus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can do away with this class at some point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moving to pydantic is my next pet project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cooldown collab?
2017075
to
c3503bc
Compare
d930c28
to
2f4c94e
Compare
we can cut a pre-release and have some alpha/uat testing done here @nicholas-lockhart I think it would be worth it if folks have time to help test? |
we should get this merged relatively soon so it doesn't rot on the vine |
Summary
type stubs and mypy compliance. also ran our standard set of pre-commit linters against everything
enables using the client in typed contexts (all of Insights) with in-editor sanity checks without doing nasty type casts or ignores:
also works for the async client, example in
examples/typed_calls.py
enabled by turning http request classes into a generic that exposes its anticipated return type. for ex:
All typing should be compatible with the versions we say we test against (3.8+), and should not cause any breaking changes for people who are not using static typing.
verifications of that claim -- i made no material changes to any of the existing tests, only the linting:
tox:
mypy:
integration tests:
these tests were already failing
vs. the run with my changes: