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.

Sign up for GitHub

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

Jump to bottom

#[pyclass] for fieldless enums #2034

Merged
merged 1 commit into from
Feb 7, 2022
Merged

#[pyclass] for fieldless enums #2034

merged 1 commit into from
Feb 7, 2022

Conversation

jovenlin0527
Copy link
Contributor

@jovenlin0527 jovenlin0527 commented Nov 30, 2021 •
edited by davidhewitt
Loading

Closes #834
Closes #2013

@jovenlin0527 jovenlin0527 force-pushed the derive-enum branch 2 times, most recently from e669680 to d4bcaf5 Compare December 1, 2021 22:56
@davidhewitt
Copy link
Member

Thanks and sorry for the delays - hoping to review this tonight.

davidhewitt
davidhewitt reviewed Dec 9, 2021
View reviewed changes
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, functionality is looking great! Really really pleased to have this for 0.16 !

Got a few comments about docs, tests, and parsing reprs.

guide/src/class.md Outdated Show resolved Hide resolved
guide/src/class.md Outdated Show resolved Hide resolved
guide/src/class.md Show resolved Hide resolved
guide/src/class.md Show resolved Hide resolved
CHANGELOG.md 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
pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved
tests/test_enum.rs Show resolved Hide resolved
birkenfeld
birkenfeld requested changes Dec 9, 2021
View reviewed changes
guide/src/class.md Outdated Show resolved Hide resolved
guide/src/class.md 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
pyo3-macros/src/lib.rs Outdated Show resolved Hide resolved
@jovenlin0527
Copy link
Contributor Author

jovenlin0527 commented Dec 10, 2021 •
edited
Loading

Thanks for the reviews! Sorry for the typos and grammatical errors,I'm not very fluent in English 😓. Hope that doesn't make this PR too difficult to review...

I've fixed all errors in docs and comments. Will fix the code later. (By the way, when I fix an error, I also mark the corresponding conversation as "resolved". That is, I use "resolve conversation" as a way to say "I think your suggestion is good and edited my PR accordingly". Is it okay or too confusing? I'm not familiar with open source practices...)

@birkenfeld
Copy link
Member

Don't worry, perfect English is not a requirement. I just happen to spot those easily 😄 Marking conversations as resolved is exactly right for "done" changes, I think.

@jovenlin0527
Copy link
Contributor Author

I have rebased this PR. I also squashed some commits. Hopefully the granularity is better than my previous PRs.

Can you review this again?

birkenfeld
birkenfeld reviewed Dec 10, 2021
View reviewed changes
Copy link
Member

@birkenfeld birkenfeld left a comment

Choose a reason for hiding this comment

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

LGTM now, but clippy and the tests are failing...

guide/src/class.md Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyclass.rs Show resolved Hide resolved
@jovenlin0527
Copy link
Contributor Author

Sorry, there have been some issues on my machine.. rustc 1.57 makes test_compile_error fail, and I didn't realize it stops other tests from running 😓. Will fix clippy later.

@jovenlin0527 jovenlin0527 force-pushed the derive-enum branch 3 times, most recently from b933efe to 720708a Compare December 10, 2021 17:10
davidhewitt
davidhewitt approved these changes Dec 18, 2021
View reviewed changes
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.

Sorry this has taken me some time to get to again. Looks great!

I have just a couple of tiny suggestions, including how to fix the MSRV job.

Also needs a rebase to fix merge conflict.

After that, let's merge this. Thank you again for this really great feature contribution!

pyo3-macros-backend/src/pyclass.rs Show resolved Hide resolved
pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved
tests/test_enum.rs Show resolved Hide resolved
@jovenlin0527 jovenlin0527 marked this pull request as draft December 22, 2021 11:58
@davidhewitt davidhewitt mentioned this pull request Dec 27, 2021
Closed
10 tasks
@davidhewitt
Copy link
Member

Thanks again for this - I've just rebased it and will merge shortly!

@davidhewitt davidhewitt linked an issue Feb 7, 2022 that may be closed by this pull request
#[pyclass] on enums MVP #2013
Closed
6 tasks
@davidhewitt davidhewitt marked this pull request as ready for review February 7, 2022 22:20
@davidhewitt davidhewitt merged commit ada3017 into PyO3:main Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamreichold adamreichold adamreichold left review comments

@birkenfeld birkenfeld birkenfeld requested changes

@davidhewitt davidhewitt davidhewitt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

#[pyclass] on enums MVP Support exporting Rust enums to Python (old school / without data)
4 participants
@jovenlin0527 @davidhewitt @birkenfeld @adamreichold