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

Revamp PyType name functions to match PEP 737 #4196

Merged
merged 5 commits into from
Jun 23, 2024

Conversation

aneeshusa
Copy link
Contributor

PyType::name uses tp_name, which is not consistent.
PEP 737 adds a new path forward,
so update PyType::name and add PyType::{module,fully_qualified_name}
to match the PEP.

Thank you for contributing to PyO3!

By submitting these contributions you agree for them to be dual-licensed under PyO3's MIT OR Apache-2.0 license.

Please consider adding the following to your pull request:

  • an entry for this PR in newsfragments - see [https://pyo3.rs/main/contributing.html#documenting-changes]
    • or start the PR title with docs: if this is a docs-only change to skip the check
  • docs to all new functions and / or detail in the guide
  • tests for all new or changed functions

PyO3's CI pipeline will check your pull request, thus make sure you have checked the Contributing.md guidelines. To run most of its tests
locally, you can run nox. See nox --list-sessions
for a list of supported actions.

@aneeshusa aneeshusa force-pushed the revamp-name-functions branch from 1ce08ad to f0ccf4c Compare May 20, 2024 22:09
@aneeshusa
Copy link
Contributor Author

Closes #4055 and #4056. Thanks @davidhewitt for helping me with this at PyCon sprints today!

@aneeshusa aneeshusa marked this pull request as ready for review May 20, 2024 22:11
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks for working on this yesterday! Overall these implementations look nice and tidy (I guess not a huge surprise given we're now better aligned with CPython) so that further convinces me that this is definitely the right approach.

It looks like the pipeline has hit some compile failures elsewhere in the codebase due to the change from Cow<str> to String. Those should hopefully be easy enough to locate and fix.

It would also be really great to add a section to migration.md to explain the PyType::name() change, please.

newsfragments/4196.changed.md Outdated Show resolved Hide resolved
src/types/typeobject.rs Outdated Show resolved Hide resolved
src/types/typeobject.rs Outdated Show resolved Hide resolved
@aneeshusa
Copy link
Contributor Author

Thanks for the review, would love help with a few of the CI failures.

guide/src/migration.md Outdated Show resolved Hide resolved
PyType::name uses `tp_name`, which is not consistent.
[PEP 737](https://peps.python.org/pep-0737/) adds a new path forward,
so update PyType::name and add PyType::{module,fully_qualified_name}
to match the PEP.
@aneeshusa aneeshusa force-pushed the revamp-name-functions branch from 665f5f6 to 4cc8714 Compare June 11, 2024 04:35
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks! I'd love to ship this in 0.22 which I'll prep soon; would it be helpful if I pushed a commit to adjust the CI edge cases?

src/types/typeobject.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

davidhewitt commented Jun 13, 2024

I pushed a couple of commits; the last one makes these functions return Bound<'py, PyString>, which I think is correct for efficiency now that all these variables already exist in the interpreter as Python strings. However to make interface changes less breaking for users I think I'd like to start with a related PR which will add comparisons and other conveniences to Bound<'py, PyString>.

@davidhewitt
Copy link
Member

Pushed that to #4245

@davidhewitt davidhewitt mentioned this pull request Jun 15, 2024
5 tasks
@davidhewitt
Copy link
Member

I think hopefully this is now ready to merge! I'm quite happy with how this has worked out :)

@davidhewitt davidhewitt force-pushed the revamp-name-functions branch from 10f1450 to c886127 Compare June 18, 2024 05:48
@davidhewitt davidhewitt force-pushed the revamp-name-functions branch from c886127 to 074310f Compare June 18, 2024 06:06
@davidhewitt
Copy link
Member

Before proceeding to merge, I'd love to hear other maintainers' opinions on the choice to change these functions to return Bound<'py, PyString>.

I think it is correct from an efficiency perspective, avoiding needless extra conversion to Rust strings in all cases except the pre-3.13 implementation of fully_qualified_name where we occasionally go the other way and build a Python string from a Rust string.

@aneeshusa
Copy link
Contributor Author

Thanks for the help David, this ended up being more complicated than I expected so I appreciate you fixing all the new compile errors that showed up after I addressed the initial ones. Looking forward to this being merged!

@davidhewitt davidhewitt mentioned this pull request Jun 20, 2024
@davidhewitt
Copy link
Member

No problem, thanks for getting this one moving! It turned into one of those PRs where as I was adjusting things it spawned new ideas how to simplify and (hopefully) reach a better end result!

@davidhewitt
Copy link
Member

If nobody raises objections before Sunday, I'll probably merge this one as-is and then proceed with publishing 0.22. 🚀

@alex
Copy link
Contributor

alex commented Jun 21, 2024

This looks good to me.

@davidhewitt davidhewitt added this pull request to the merge queue Jun 22, 2024
Merged via the queue into PyO3:main with commit c67625d Jun 23, 2024
41 checks passed
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.

4 participants