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

Make the usage of the module typing consistent #611

Closed
alallema opened this issue Nov 29, 2022 · 2 comments · Fixed by #625
Closed

Make the usage of the module typing consistent #611

alallema opened this issue Nov 29, 2022 · 2 comments · Fixed by #625
Labels
good first issue Good for newcomers maintenance Anything related to maintenance (CI, tests, refactoring...)

Comments

@alallema
Copy link
Contributor

alallema commented Nov 29, 2022

In some parts of the SDK, the typing module is used but not everywhere so that in some places the dict type is used and in others the Dict type.
It would be nice to make the whole thing coherent and make it easier to use the module everywhere.

@alallema alallema added good first issue Good for newcomers maintenance Anything related to maintenance (CI, tests, refactoring...) labels Nov 29, 2022
@sanders41
Copy link
Collaborator

I think you are referring to Dict in this file. The reason it uses Dict instead of dict is Pydantic won't work with all the supported python versions using dict (can't use __future__ annotations).

You could go back to using from typing import Dict everywhere. Technically Union in this file would fall under this also, so str | None (or any union type like this) would become Union[str, None]. In addition any new types added to a Pydantic model that require import from typing, List, Tuple, etc. the same thing would need to be done.

@alallema
Copy link
Contributor Author

Hi @sanders41,

I think you are referring to Dict in this file.

Yes totally

You could go back to using from typing import Dict everywhere. Technically Union in this file would fall under this also, so str | None (or any union type like this) would become Union[str, None].

Yes I have read some debates about what was best to use:
Union[str, None]
Optional[str]
str | None
But I don't have an opinion on the matter I just wish that all the code was written in a homogeneous way and compatible with all versions.

Thanks for your advice ❤️

@bors bors bot closed this as completed in 8307786 Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers maintenance Anything related to maintenance (CI, tests, refactoring...)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants