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

Make growing types non exhaustive #52

Merged
merged 4 commits into from
Jan 2, 2024
Merged

Conversation

faern
Copy link
Member

@faern faern commented Jan 2, 2024

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-tcp does 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 Options struct? 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 the statsd feature 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.x after that even if we add optional option fields or error variants, something we cannot if we don't merge this PR.


This change is Reviewable

faern added 2 commits January 2, 2024 14:43
Allows adding more error variants without it being a breaking change
@faern faern requested a review from dlon January 2, 2024 13:49
@faern faern force-pushed the make-growing-types-non_exhaustive branch from 4ebf16e to c60b3e9 Compare January 2, 2024 13:51
Copy link
Member Author

@faern faern left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions

@faern faern merged commit 517b970 into main Jan 2, 2024
13 checks passed
@faern faern deleted the make-growing-types-non_exhaustive branch January 2, 2024 14:08
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.

2 participants