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

Enum naming improvements #2457

Merged
merged 3 commits into from Jun 22, 2022
Merged

Conversation

yodaldevoid
Copy link
Contributor

This improves the interaction between #[pyclass(name)] and enums.

First, the macro-defined name for the enum is used in the enum's __repr__ implementation so that everything is kept consistent from the Python point of view.

Secondly, #[pyo3(name)] is now supported on enum variants.

@yodaldevoid yodaldevoid marked this pull request as ready for review June 16, 2022 19:53
@yodaldevoid yodaldevoid force-pushed the enum_naming_improvements branch 2 times, most recently from f36cd12 to b18d9cf Compare June 17, 2022 14:13
Copy link
Member

@messense messense left a comment

Choose a reason for hiding this comment

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

I think it's better to name some struct and fns enum variant instead of just variant.

pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved
@yodaldevoid
Copy link
Contributor Author

I think it's better to name some struct and fns enum variant instead of just variant.

I've applied the naming suggestions.

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 very much for this, and sorry I have been slow to review.

Implementation is very complete and docs are great 👍

Just one tiny tweak I'd like to see to contain the options inside the PyClassEnumVariant struct (if it's straightforward to do so). Also need to rebase to fix the CHANGELOG merge conflict. Then let's merge!

pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved
@yodaldevoid
Copy link
Contributor Author

I've incorporated the suggested changes and fixed the merge conflict in the CHANGELOG.

@messense
Copy link
Member

error: explicit lifetimes given in parameter types where they could be elided (or replaced with `'_` if needed by type declaration)
--> pyo3-macros-backend/src/pyclass.rs:337:5
|
337|     fn python_name<'b>(&'b self) -> Cow<'b, syn::Ident> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::needless-lifetimes` implied by `-D warnings`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes

There is a clippy warning needs to be addressed.

@yodaldevoid
Copy link
Contributor Author

There is a clippy warning needs to be addressed.

Apologies, I forgot to run clippy before pushing. This should now be addressed.

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.

Looks great - many thanks again!

@davidhewitt davidhewitt merged commit 510c126 into PyO3:main Jun 22, 2022
@yodaldevoid yodaldevoid deleted the enum_naming_improvements branch July 15, 2022 19:36
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.

None yet

3 participants