-
-
Notifications
You must be signed in to change notification settings - Fork 680
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
🔥 Remove unused functionality from _typing.py
file
#805
Conversation
📝 Docs preview for commit 0f77a8a at: https://2f3710b1.typertiangolo.pages.dev |
It was only used to import the `get_args` and `get_origin` methods, but we can import them from `typing_extensions` directly.
0f77a8a
to
f5f5304
Compare
📝 Docs preview for commit f5f5304 at: https://bf7d0262.typertiangolo.pages.dev |
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.
Hi, thanks for the contribution! It would be great to clean this up.
As you're deleting quite a bit of code though, can you elaborate a bit more on why it's all become obsolete?
This file was extracted from pydantic, before pydantic removed it with its support of python3.6 As mentioned above, the only 2 methods used in this big file were My guess is that this file was useful for python3.6, but now it's not, as libraries like |
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.
Currently, main.py
uses the following functionality:
from ._typing import get_args, get_origin, is_literal_type, is_union, literal_values
Further, the _typing.py
file has evolved from supporting 3.6-specific functionality to supporting other functions that may differ in Python versions 3.8 or 3.10 etc. As such, simply removing this feels too risky to me.
Instead, I would suggest to merge my PR #850 which only cleans up Python 3.6 related code, and go from there. I'll leave the final decision with Tiangolo though.
Either way - thanks again for the contribution! 🙏
Hello @svlandeg ! I understand your concerns. At the time of my PR only two methods were imported and used from the However, I see new developments have changed that situation since then, and so it would require more analysis, and the solution might not be just as simple anyway. I do believe we should aim to drop this file and favor more battle tested alternatives like the |
📝 Docs preview for commit 21b23f6 at: https://10110fd5.typertiangolo.pages.dev |
_typing.py
file_typing.py
file
📝 Docs preview for commit 51b8712 at: https://6554f2b6.typertiangolo.pages.dev |
📝 Docs preview for commit 78dec75 at: https://4dfc6bf4.typertiangolo.pages.dev |
📝 Docs preview for commit 823fcd4 at: https://0fe6f379.typertiangolo.pages.dev |
@ivantodorovich: my counter proposal, reflected by the current state of this PR:
But, instead of entirely deleting the file:
This will make the file much more manageable in the future, and it'll be easier to refactor functionality further when/if we need it. |
Thanks @svlandeg ! That looks great ❤️ |
Great, thank you @ivantodorovich! ☕ And thanks @svlandeg for all the work and help here. 🙇 🍰 |
It was only used to import the
get_args
andget_origin
methods, but we can import them fromtyping_extensions
directly.