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

remove __non_exhaustive members, add non_exhaustive attribute instead #1848

Merged
merged 1 commit into from
Jul 9, 2022

Conversation

FauconFan
Copy link
Contributor

As stated in #1835, I deleted every non_exhaustive members and added the #[non_exhaustive] attribute.
There are two more structs that may benefit from this: the Chapter struct and the Theme struct. The remaining structs have either a private member either all public members and doesn't deserve the attribute (to my opinion).
I could have put this attribute to enums as well, but this and adding attribute to the two structs above would be a breaking change. The page reference for non_exhaustive attribute. So I let this up to review.

@Dylan-DPC Dylan-DPC merged commit 42d6fd5 into rust-lang:master Jul 9, 2022
@Dylan-DPC
Copy link
Member

Thanks

@FauconFan FauconFan deleted the remove_non_exhaustive branch July 9, 2022 14:21
@ehuss
Copy link
Contributor

ehuss commented Jul 9, 2022

Sorry, I'm maybe a bit confused. This is a breaking change, and #1835 explicitly says this should be done for the 0.5 release. Unless I'm misunderstanding what the intent was here, can you please revert this change?

@FauconFan
Copy link
Contributor Author

Well, maybe i'm wrong, but adding the #[non_exhaustive] attribute is not a breaking change if there were already private members right ?
I was talking about breaking change for other structs / enums.

@ehuss
Copy link
Contributor

ehuss commented Jul 9, 2022

It is unfortunately exposed to preprocessors. #1571 contains some more information.

@FauconFan
Copy link
Contributor Author

Oh yes indeed, sorry for that, it makes sense, now !

@FauconFan
Copy link
Contributor Author

Well maybe it makes sense to create a new branch for release 0.5.0, then ?

@Dylan-DPC
Copy link
Member

Ah my bad.

Nah i wouldn't recommend having a another branch - having 2 active branches makes it annoying to maintain and keep up to date. I would prefer holding this or having an open pr till 0.5 lands

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.

3 participants