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

🔥 Remove unused functionality from _typing.py file #805

Merged
merged 7 commits into from
Aug 28, 2024

Conversation

ivantodorovich
Copy link
Contributor

It was only used to import the get_args and get_origin methods, but we can import them from typing_extensions directly.

Copy link

📝 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.
Copy link

📝 Docs preview for commit f5f5304 at: https://bf7d0262.typertiangolo.pages.dev

Copy link
Member

@svlandeg svlandeg left a 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?

@ivantodorovich
Copy link
Contributor Author

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
The header said:
Copied from pydantic 1.9.2 (the latest version to support python 3.6.)

As mentioned above, the only 2 methods used in this big file were get_args and get_origin, which can easily be imported from the maintained typing_extensions package.

My guess is that this file was useful for python3.6, but now it's not, as libraries like typing_extensions or even the standard library have grown and include other alternatives

@svlandeg svlandeg self-assigned this Aug 5, 2024
Copy link
Member

@svlandeg svlandeg left a 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! 🙏

@svlandeg svlandeg removed their assignment Aug 20, 2024
@ivantodorovich
Copy link
Contributor Author

Hello @svlandeg !

I understand your concerns.

At the time of my PR only two methods were imported and used from the _typing.py module: get_args and get_origin, and simply using their typing_extensions counterparts worked perfectly fine.

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 typing_extensions module, which we are already using. Besides the maintenance burden, I believe it encourages contributors to keep using and improving the methods defined here instead of analyzing those of typing_extensions, making the situation harder to solve.

@svlandeg svlandeg reopened this Aug 26, 2024
@svlandeg svlandeg self-assigned this Aug 26, 2024
@svlandeg svlandeg marked this pull request as draft August 26, 2024 12:56
Copy link

📝 Docs preview for commit 21b23f6 at: https://10110fd5.typertiangolo.pages.dev

@svlandeg svlandeg changed the title 🔥 Remove _typing.py file 🔥 Remove unused functionality from _typing.py file Aug 26, 2024
Copy link

📝 Docs preview for commit 51b8712 at: https://6554f2b6.typertiangolo.pages.dev

Copy link

📝 Docs preview for commit 78dec75 at: https://4dfc6bf4.typertiangolo.pages.dev

Copy link

📝 Docs preview for commit 823fcd4 at: https://0fe6f379.typertiangolo.pages.dev

@svlandeg
Copy link
Member

svlandeg commented Aug 26, 2024

@ivantodorovich: my counter proposal, reflected by the current state of this PR:

  • Import get_args and get_origin from typing_extensions as you suggested

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.

@svlandeg svlandeg removed their assignment Aug 26, 2024
@svlandeg svlandeg marked this pull request as ready for review August 26, 2024 14:28
@ivantodorovich
Copy link
Contributor Author

Thanks @svlandeg !

That looks great ❤️

@tiangolo
Copy link
Member

Great, thank you @ivantodorovich! ☕

And thanks @svlandeg for all the work and help here. 🙇 🍰

@tiangolo tiangolo merged commit 67a9eee into fastapi:master Aug 28, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants