-
Notifications
You must be signed in to change notification settings - Fork 86
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
Mark lots of enums #[non_exhaustive]
#124
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
src/service.rs
line 950 at r1 (raw file):
/// Enum describing the reason of a SessionChange event #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] #[non_exhaustive]
This seems to be exhaustive right now.
37df250
to
f215e4a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 3 files reviewed, all discussions resolved (waiting on @dlon)
src/service.rs
line 950 at r1 (raw file):
Previously, dlon (David Lönnhager) wrote…
This seems to be exhaustive right now.
Ok! Yes, seems you are right. I removed that commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
#[non exhaustive]
#[non_exhaustive]
For the upcoming
0.7.0
release we have a lot of breaking changes due to adding variants to enums. Lots of these enums are probably still not exhaustive. To mitigate having to make breaking changes for small stuff in the future, I mark them as#[non_exhaustive]
. If you think any of these actually are fully exhaustive now, and/or if you think an addition to one of them actually should be a breaking change, then please give that as feedback.I want this merged before the
0.7.0
release.This change is