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

Use serde derive macros on Interface #174

Merged
merged 1 commit into from
Apr 17, 2024
Merged

Conversation

DataTriny
Copy link
Collaborator

Relates to #157

There are more complex cases where avoiding the use of the strum crate will require more design changes, so I'm addressing this one first.

@DataTriny
Copy link
Collaborator Author

The codecov CI job is failing, not because of a decrease in coverage though.

FWIW I have removed some tests related to the manual serialization/deserialization of the Interface type so I expect the coverage to decrease a bit.

@TTWNO
Copy link
Member

TTWNO commented Apr 14, 2024

The codecov CI job is failing, not because of a decrease in coverage though.

No problem. It's always a bit tempermental.

}
}

impl std::fmt::Display for Interface {
Copy link
Member

Choose a reason for hiding this comment

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

Why have you removed the Display implementation? Does Serialize auto-implement this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the constants so as to not have duplicate names, so we'd have to write the string literals inside the Display impl in order to keep this. But what's the added value of having this? It was only used in tests that I removed, I think the Debug impl is enough for users.

@DataTriny DataTriny mentioned this pull request Apr 14, 2024
@TTWNO
Copy link
Member

TTWNO commented Apr 17, 2024

This removes a ton of manual code. I'm merging.

@TTWNO TTWNO merged commit ebbee49 into main Apr 17, 2024
10 of 11 checks passed
@TTWNO TTWNO deleted the serde-derive-on-interface branch April 17, 2024 17:42
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