Skip to content

#[pyclass] for fieldless enums#2034

Merged
davidhewitt merged 1 commit intoPyO3:mainfrom
jovenlin0527:derive-enum
Feb 7, 2022
Merged

#[pyclass] for fieldless enums#2034
davidhewitt merged 1 commit intoPyO3:mainfrom
jovenlin0527:derive-enum

Conversation

@jovenlin0527
Copy link
Contributor

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

Closes #834
Closes #2013

adamreichold
adamreichold reviewed Nov 30, 2021
View reviewed changes
@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

davidhewitt commented Dec 9, 2021

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.

birkenfeld
birkenfeld requested changes Dec 9, 2021
View reviewed changes
@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

birkenfeld commented Dec 10, 2021

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

jovenlin0527 commented Dec 10, 2021

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

@jovenlin0527
Copy link
Contributor Author

jovenlin0527 commented Dec 10, 2021

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!

@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

davidhewitt commented Feb 7, 2022

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

@birkenfeld birkenfeld birkenfeld requested changes

@adamreichold adamreichold adamreichold left review comments

@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