encode / starlette

The little ASGI framework that shines. 🌟
https://www.starlette.io/
BSD 3-Clause "New" or "Revised" License
10.31k stars 949 forks source link

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

Closed mgorny closed 4 months ago

mgorny commented 4 months ago

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

Kludex commented 4 months ago

Can we link the CPython PR here?

I don't understand why change this behavior.

mgorny commented 4 months ago

Can we link the CPython PR here?

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

mgorny commented 4 months ago

Can we link the CPython PR here?

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

The change in question is python/cpython@49e5740135670e04ae6da7e6f52dbe380655e0f1. 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 commented 4 months ago

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 commented 4 months ago

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 https://github.com/python/cpython/issues/122087 and drop the downstream patch if this ends up being handled in CPython 3.13.0rc1.

mgorny commented 4 months ago

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

Kludex commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Thanks a lot!