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

gh-128661: Remove DeprecationWarning in evaluate_forward_ref #128930

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Jan 17, 2025

It doesn't make sense to use a deprecation for evaluate_forward_ref,
as it is a new function in Python 3.14 and doesn't have compatibility
guarantees.

I considered making it throw an error if type_params it not passed and
there is no owner. However, I think this is too unfriendly for users. The
case where this param is really needed is fairly esoteric and I don't think
this case is worth the pain of forcing users to write "type_params=()".


📚 Documentation preview 📚: https://cpython-previews--128930.org.readthedocs.build/

It doesn't make sense to use a deprecation for evaluate_forward_ref,
as it is a new function in Python 3.14 and doesn't have compatibility
guarantees.

I considered making it throw an error if type_params it not passed and
there is no owner. However, I think this is too unfriendly for users. The
case where this param is really needed is fairly esoteric and I don't think
this case is worth the pain of forcing users to write "type_params=()".
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @AlexWaygood who reviewed my PR.

@@ -1024,7 +1024,7 @@ def evaluate_forward_ref(
owner=None,
globals=None,
locals=None,
type_params=_sentinel,
type_params=None,
Copy link
Member

@sobolevn sobolevn Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is that it would be rather easy to miss. This function has multiple parameters, __type_params__ is a new thing, which not many people are familiar with.

Proposed API makes it easy to miss.

And when missed, simple code like -> T and evaluate_forward_ref('T', format=VALUE) (instead of evaluate_forward_ref('T', format=VALUE, type_params=func.__type_params__)) can raise an error.

On the other hand, type_params is indeed optional in a sense, that users can't even provide type_params in all cases.

I have very mixed feelings about it :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your example the user should rather pass owner=. Pointing them to use type_params= only makes the API more complicated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I am not yet fully familiar with this new API. Why do we even need type_params then, if it can be replaced with owner=? 🤔

@AlexWaygood
Copy link
Member

Agree with @sobolevn. A DeprecationWarning obviously doesn't make sense, since it's a new function (sorry for missing that in the other PR). But I think you'll only know that you need to pass type_params explicitly when it starts failing with an inexplicable error that's really hard to debug. This feels like too much of a footgun to me :/

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