Make growing types non exhaustive #52
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Allows adding more error variants without it being a breaking change
faern
force-pushed
the
make-growing-types-non_exhaustive
branch
from
4ebf16e to
c60b3e9
Compare
faern
commented
Jan 2, 2024
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @dlon)
src/tcp2udp.rs line 24 at r1 (raw file):
/// This struct is `non_exhaustive` in order to allow adding more optional fields without /// being considered breaking changes. So you need to create an instance via [`Options::new`]. #[derive(Debug, Clone)]
The Clone part here is not important. I just felt like there is no reason for it not to be clonable, and it might make it more ergonomic to use.
src/tcp2udp.rs line 48 at r1 (raw file):
/// Host to send statsd metrics to. #[cfg_attr(feature = "clap", clap(long))] pub statsd_host: Option<SocketAddr>,
Oops. Forgot to set this to pub in the previous PR. This was clearly a bug, since users of the library must be able to set this field.
dlon
approved these changes
Jan 2, 2024
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was about to bump the version and release. Given that we have added an error variant to an enum and a field to a struct it's technically a breaking change even if the feature is off by default... At least I think it should count as that 🤔 Because even if you as a direct user of
udp-over-tcpdoes not activate the feature, a transitive dependency might....The fix for the error enum is easy, make them
#[non_exhaustive]. I think this is a pretty good default for error enums to begin with. Because it allows adding more error cases without it being a breaking change...But what about the
Optionsstruct? I figured the best might be to also make it#[non_exhaustive]and provide facilities for users to construct it without hitting compilation errors both if others enable thestatsdfeature OR if we add other optional fields later.We still need to bump the version in a breaking fashion after this PR (to 0.4.0) but these changes will allow us to stay on
0.4.xafter that even if we add optional option fields or error variants, something we cannot if we don't merge this PR.This change is