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

replace derivative crate with derive_more crate #439

Merged
merged 4 commits into from
Oct 17, 2024

Conversation

YJDoc2
Copy link
Contributor

@YJDoc2 YJDoc2 commented Oct 16, 2024

This PR carries over from #437 and changes the derivative crate to the derive_more crate. This is particularly for the Debug derives which were bandwhich's main usecase for derivative .

  • Removed debug_fn (formatting fn) in code, as it only adds a fixed string, so directly used that string in the macro.
  • Used Rust's normal Default derive, as our main use case for it was setting the default of Verbosity . But Verbosity itself is a simple struct having two u8 fields and a PhantomData , which also does a derive(Default) as the default for u8 is 0, the "normal" Default should be same as Verbosity::new(0, 0) .

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>
@cyqsimon
Copy link
Collaborator

Thanks for the PR; code looks pretty good. Two comments:

  1. Seems like derive_more requires 1.75.0. I'm okay with it, but we do need to bump MSRV in Cargo.toml.
  2. Please add a changelog entry.

Then we're good to merge.

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>
Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>
Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>
@YJDoc2
Copy link
Contributor Author

YJDoc2 commented Oct 17, 2024

Seems like derive_more requires 1.75.0. I'm okay with it, but we do need to bump MSRV in Cargo.toml.

Bumped rust-version in Cargo.toml

Please add a changelog entry.

I have added two entries, one for crate change and once for msrv bump, both pointing to this PR, is that ok? Should I combine them in just one entry?

Just a nit-pick: is this something your formatter did automatically? I think by default RA likes to order the imports in this order: std, foreign, crate.

Fixed, please check.

@cyqsimon
Copy link
Collaborator

I have added two entries, one for crate change and once for msrv bump, both pointing to this PR, is that ok? Should I combine them in just one entry?

Yeah either is fine really. Merging.

@cyqsimon cyqsimon merged commit 01e5fed into imsnif:main Oct 17, 2024
35 of 36 checks passed
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