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: make PyType::name consistent with __name__ and add PyType::module #4056

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

Conversation

wyfo
Copy link
Contributor

@wyfo wyfo commented Apr 7, 2024

fixes #4055

@adamreichold
Copy link
Member

I am not particularly keen on the additional complexity and would be prefer to limit to fixing inconsistencies between builds with and without abi3.

@davidhewitt
Copy link
Member

As well as efficiency, it looks like PEP 737 makes it clear that exposing tp_name is a bad idea. Even though we just changed the way these worked in 0.21, I think it's probably correct to change PyType::name to just match __name__ in 0.22. Sigh.

However I agree with the complexity concern here.

Given that PEP 737 adds a new PyType_GetFullyQualifiedName function to Python 3.13, I suggest what we should do is not have PyType::full_name at all, and instead add PyType::fully_qualified_name which will be __module__.__qualname__ as per PEP 737. We can even use the API function on new Python versions.

That way we have a set of functions which match Python well, and should be relatively simple to implement:

  • PyType::name for __name__
  • PyType::qualname for __qualname__
  • PyType::module for __module__
  • PyType::fully_qualified_name for __module__.__qualname (as per PEP 737 rules of when to skip __module__).

What do you both think of this?

@wyfo
Copy link
Contributor Author

wyfo commented Apr 12, 2024

I prefer to be aligned with Python dunder attributes, I find this more intuitive.

@davidhewitt
Copy link
Member

@adamreichold as you already raised a complexity concern, what do you think of what I suggest above?

@davidhewitt
Copy link
Member

Ping @adamreichold (and other maintainers), what do you think of this? I still believe we errored in the design for 0.21 and this correction is worthwhile despite the churn.

@adamreichold
Copy link
Member

Ping @adamreichold (and other maintainers), what do you think of this? I still believe we errored in the design for 0.21 and this correction is worthwhile despite the churn.

Yes, I agree that PEP compliance is preferable and outweighs the perceived performance benefits of borrowing tp_name. I just did not find the spare time to look into the implementation yet. Sorry for that.

@davidhewitt
Copy link
Member

No problem, I am happy to also try to implement (or maybe @wyfo is willing to rework this PR). Mostly just wanted to make sure the design blocker was removed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Borrowed<PyType>::name is not consistent
3 participants