-
Notifications
You must be signed in to change notification settings - Fork 206
SG-39414 Add type annotations #422
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
| """ | ||
|
|
||
| import datetime | ||
| from typing import 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.
I don't like using things from outside without having a package/module name.
| from typing import Any | |
| import typing |
Then use typing.Any.
If you are looking for a short version, we can always aggree on
import typing as tpThere 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 is a subjective personal preference, and we cannot enforce this. I agree on being explicit and importing the objects specifically.
|
|
||
| @classmethod | ||
| def get_schemas(cls, schema_path, schema_entity_path): | ||
| def get_schemas(cls, schema_path: str, schema_entity_path: str) -> tuple: |
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.
return value: tuple of what? Can we define string/int/dict/... ?
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'll leave it as is, since it's a unit testing helper method. Not everything in Mockgun has been typed so far.
| direction: str | ||
|
|
||
|
|
||
| class BaseEntity(TypedDict, total=False): |
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.
what is this total=False syntax? I never saw that
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 means that we don't expect to require all the defined fields.
Closes #393
Thanks @chadrik