-
Notifications
You must be signed in to change notification settings - Fork 204
SG-39414 Add type annotations #393
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
base: master
Are you sure you want to change the base?
Conversation
cf7e8db
to
ff3a639
Compare
I don't think these CI failures are related to my changes. |
30eee33
to
85c0f50
Compare
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.
It would be cool to add a blank py.typed
marker file so tools like mypy know that this package has type hints!
https://peps.python.org/pep-0561/#packaging-type-information
I believe we'd also need to update setup.py to include that in the package data.
|
||
class BaseEntity(TypedDict, total=False): | ||
id: int | ||
type: str |
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 added some TypedDicts
to try to fill in some of the required dictionary fields. Let me know if you see anything wrong here.
entity_type: str, | ||
data: dict[str, Any], | ||
return_fields: Optional[list] = None, | ||
) -> dict[str, Any]: |
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.
This should be changed to the BaseEntity
TypedDict
Anything else I can do to help this along? |
Resolves #307 and #233
When running
mypy
against the code there are still some errors, but the annotations should provide some good value to users without any real downsides.