Skip to content

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

Merged
Kludex merged 1 commit intoKludex:masterfrom
mgorny:py313-partial
Jul 24, 2024
Merged

Fix routing.get_name() not to assume all routines have __name__#2648
Kludex merged 1 commit intoKludex:masterfrom
mgorny:py313-partial

Conversation

@mgorny
Copy link
Contributor

@mgorny mgorny commented Jul 21, 2024 •
edited
Loading

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
Owner

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
View reviewed changes
Copy link
Owner

@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?

@mgorny mgorny mentioned this pull request Jul 21, 2024
Closed
Kludex
Kludex previously approved these changes Jul 21, 2024
View reviewed changes
Copy link
Owner

@Kludex Kludex left a comment •
edited
Loading

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
Owner

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
Contributor

musicinmybrain commented Jul 23, 2024

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.

@mgorny
Fix routing.get_name() not to assume all routines have __name__
68ad771 
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 Kludex#2638
@Kludex
Copy link
Owner

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 Kludex:master Jul 24, 2024
@Kludex
Copy link
Owner

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

Reviewers

@Kludex Kludex Kludex approved these changes

Assignees

No one assigned

Labels

hold Don't merge it

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@mgorny @Kludex @musicinmybrain