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

Fix routing.get_name() not to assume all routines have __name__ #2648

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Jul 21, 2024

Summary

Fix routing.get_name() to use the __name__ attribute only if it is actually present, rather than assuming that all routine and class types have it, and use the fallback to class name otherwise. This is necessary for functools.partial() that doesn't have a __name__ attribute, but is recognized as a routine starting with Python 3.13.0b3. Since for older versions of Python, it would have used the fallback anyway, this doesn't really change the behavior there.

Fixes #2638

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change. ← this is covered by existing tests already
  • I've updated the documentation accordingly. ← I don't think any changes are necessary

@Kludex
Copy link
Sponsor Member

Kludex commented Jul 21, 2024

Can we link the CPython PR here?

I don't understand why change this behavior.

Kludex
Kludex previously approved these changes Jul 21, 2024
Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Oh well... Seems okay anyway. If it has a __name__, use it, otherwise use __class__.__name__.

@Kludex Kludex dismissed their stale review July 21, 2024 12:28

We should add some tests anyway.

@mgorny
Copy link
Contributor Author

mgorny commented Jul 21, 2024

Can we link the CPython PR here?

Do you mean the commit that changed partial() behavior?

@mgorny
Copy link
Contributor Author

mgorny commented Jul 21, 2024

Can we link the CPython PR here?

Do you mean the commit that changed partial() behavior?

The change in question is python/cpython@49e5740. Actually, I'm not sure if inspect.isroutine() change was intentional or an accidental side-effect, I'll file a bug there too, just in case.

We should add some tests anyway.

What kind of tests do you have in mind?

Kludex
Kludex previously approved these changes Jul 21, 2024
Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

There's actually a test called test_route_name that tests this code.

All good here. Thanks @mgorny :)

@Kludex
Copy link
Sponsor Member

Kludex commented Jul 21, 2024

But... To be honest... If it was unintended on CPython's side, I'd prefer to wait for them to revert the unintended changes.

@musicinmybrain
Copy link

Thanks for this PR. It works nicely as a downstream patch in Fedora Rawhide, which allows me to keep building and updating Starlette. I’ll monitor python/cpython#122087 and drop the downstream patch if this ends up being handled in CPython 3.13.0rc1.

@Kludex Kludex added the hold Don't merge it label Jul 23, 2024
@Kludex Kludex dismissed their stale review July 23, 2024 14:45

Looks like CPython is going to fix the issue.

@mgorny
Copy link
Contributor Author

mgorny commented Jul 24, 2024

@Kludex, upstream confirmed that:

Yes, not all method descriptors (and therefore not all routines) have the __name__ attribute. This always was so, you always should have a fallback.

So I think we should merge this anyway.

Fix `routing.get_name()` to use the `__name__` attribute only if it is
actually present, rather than assuming that all routine and class types
have it, and use the fallback to class name otherwise.  This is
necessary for `functools.partial()` that doesn't have a `__name__`
attribute, but is recognized as a routine starting with Python 3.13.0b3.
Since for older versions of Python, it would have used the fallback
anyway, this doesn't really change the behavior there.

Fixes encode#2638
@Kludex
Copy link
Sponsor Member

Kludex commented Jul 24, 2024

@Kludex, upstream confirmed that:

Yes, not all method descriptors (and therefore not all routines) have the __name__ attribute. This always was so, you always should have a fallback.

So I think we should merge this anyway.

Well... But they are fixing it for functools.partial, right?

@mgorny
Copy link
Contributor Author

mgorny commented Jul 24, 2024

Well... But they are fixing it for functools.partial, right?

Yes, they're adding a workaround for Python 3.13. However, it will fail again in 3.14.

@Kludex Kludex merged commit 07427f8 into encode:master Jul 24, 2024
5 checks passed
@Kludex
Copy link
Sponsor Member

Kludex commented Jul 24, 2024

Well... But they are fixing it for functools.partial, right?

Yes, they're adding a workaround for Python 3.13. However, it will fail again in 3.14.

Well... Then let's go. 😅

Thanks @mgorny 🙏

@mgorny mgorny deleted the py313-partial branch July 24, 2024 08:30
@mgorny
Copy link
Contributor Author

mgorny commented Jul 24, 2024

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold Don't merge it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python 3.13.0b3: AttributeError: 'functools.partial' object has no attribute '__name__'
3 participants