Impl Display rather than ToString#663
Merged
Merged
Conversation
Updated the 3 places where ToString impls were generated (in `type_entry.rs`) and replaced them with an equivalent Display impl. ToString is implemented for all types that implement Display and for this reason the docs recommend not to implement ToString directly [0] and is actually linted against by clippy in the default groups [1]. The snapshots were updated by running `EXPECTORATE=overwrite cargo test --workspace` and then afterwards I did a brief manual verification that the generated results seemed sensible. Updated the README for `cargo-typify` to switch out the ToString impl for the Display version. Since the output in the README is a simplified version of the real output (the real output has a lot more fields and noise), I manually added the Display impl in the style of the rest of the example (nb. the Display impl does correctly appear in the real output). 0: https://doc.rust-lang.org/std/string/trait.ToString.html 1: https://rust-lang.github.io/rust-clippy/master/index.html#/to_string_trait_impl
ahl
approved these changes
Sep 25, 2024
Collaborator
ahl
left a comment
ahl
left a comment
There was a problem hiding this comment.
This looks great; nice and simple. Much appreciated.
Collaborator
|
Note to myself: I tested this with oxide.rs and all tests passed. There were no lingering |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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've been getting clippy warnings in my project that includes generated cargo-typify code due to the ToString impl so I decided to make a quick PR to fix it. I noticed #198 is the relevant issue. If you'd rather implement this slightly differently I'm more than happy to make substantial changes, this was just a quickly thrown together fix.
Description
Updated the 3 places where ToString impls were generated (in
type_entry.rs) and replaced them with an equivalent Display impl. ToString is implemented for all types that implement Display and for this reason the docs recommend not to implement ToString directly [0] and is actually linted against by clippy in the default groups [1].The snapshots were updated by running
EXPECTORATE=overwrite cargo test --workspaceand then afterwards I did a brief manual verification that the generated results seemed sensible.Updated the README for
cargo-typifyto switch out the ToString impl for the Display version. Since the output in the README is a simplified version of the real output (the real output has a lot more fields and noise), I manually added the Display impl in the style of the rest of the example (nb. the Display impl does correctly appear in the real output).0: https://doc.rust-lang.org/std/string/trait.ToString.html
1: https://rust-lang.github.io/rust-clippy/master/index.html#/to_string_trait_impl