search

oxidecomputer / typify

compiler from JSON Schema into idiomatic Rust types
Apache License 2.0
451 stars 60 forks source link

Impl Display rather than ToString #663

Closed louisdewar closed 1 month ago

louisdewar commented 2 months ago

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 --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 commented 1 month ago

Note to myself: I tested this with oxide.rs and all tests passed. There were no lingering ToString impls. There were other non-::-prefixed uses of std but we can deal with that in another PR