Skip to content

misc: Make error enums unconditionally non_exhaustive#652

Merged
jplatte merged 4 commits into
ruma:mainfrom
DevinR528:errors-nonexh-enums
Jul 2, 2021
Merged

misc: Make error enums unconditionally non_exhaustive#652
jplatte merged 4 commits into
ruma:mainfrom
DevinR528:errors-nonexh-enums

Conversation

@DevinR528

Copy link
Copy Markdown
Member

No description provided.

Comment thread crates/ruma-federation-api/src/query/get_profile_information/v1.rs
Comment thread crates/ruma-push-gateway-api/src/send_event_notification/v1.rs
Comment thread crates/ruma-serde/src/canonical_json.rs Outdated
/// Errors returned during serializing to `application/x-www-form-urlencoded`.
#[derive(Clone, Debug, PartialEq, Eq)]
#[non_exhaustive]
pub enum Error {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I kinda dislike this being an enum at all, but that's something that can be refactored separately.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What were you thinking?

Comment on lines 16 to 17
#[allow(clippy::exhaustive_enums)]
pub enum EventFormatVersion {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not make this one non-exhaustive?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So I wasn't sure about this or below but my idea was there this would always be a breaking change in the spec, and that it might be better to have an obvious break from version to version 🤷

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The whole purpose of room versions is being able to change things without breaking existing code. Why would adding to this inherently be a breaking change?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Would the spec not semver bump? I guess I'm not sure about ruma's semver guarantees with respect to the matrix spec changes and their versioning? This probably kinda relates to my question about non_exhaustive enums with a _Custom variant and forward compat? If I had a better idea of how/when we want to make breaking changes.

Comment on lines 27 to 28
#[allow(clippy::exhaustive_enums)]
pub enum StateResolutionVersion {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same question here.

@DevinR528 DevinR528 force-pushed the errors-nonexh-enums branch from 1971658 to 1d95327 Compare June 29, 2021 23:28
@DevinR528

Copy link
Copy Markdown
Member Author

I made both of the state-res RoomVersion structs non_exhaustive so I think this PR is ready unless you want me to mess with the enum urlencode::ser::Error?

@jplatte jplatte merged commit 94ddfc2 into ruma:main Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants