-
Notifications
You must be signed in to change notification settings - Fork 158
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
Derive Debug trait for all structs and enums #504
Conversation
This PR is unreasonably wide reaching. What is it exactly that you're trying to accomplish? |
The core motivation was to within a library (Ratatui) to be able to centrally define, describe and store After implementing those, I decided that it probably made sense to implement it for everything (per https://rust-lang.github.io/api-guidelines/interoperability.html#c-common-traits) (I'm assuming this was a mistake now and I should have checked). Several of the items where deriving Debug were problematic, and I went with what seemed to be the right approach of manual implementations / adding a Debug constraint to the traits. Are there any particular parts of this PR that fall into the unreasonable bucket more than others, or is your perspective more that you're unconvinced of the usefulness of generally implement Debug in this crate? My perspective is that implementing Debug is generally a useful thing to do on any item that would potentially ever need to be stored in another item. Is that something you might have differences of opinion on? |
Overall, I think just blanket deriving My two no-nos here:
For the first one, this mostly just strikes me as clutter. If there was a strong need to have debug output, they'd have For the second one, it doesn't make sense to make traits a supertrait of common traits unless they actually require that trait's functionality for their own behavior (i.e. the supertrait's effective behavior is the sum of the subtraits) or they would otherwise solve a large ergonomics issue, like having In this case, neither of those scenarios is at play, and more tactically, this would only matter if you're holding a So, all of that said: if we remove the derives from the binary crates, and from traits, I'd be OK with merging the rest. |
1f9878e
to
9f9ab78
Compare
Makes sense. I've removed the derives from the metrics-observer and benches, and removed the trait changes. Clippy lints are unrelated to this change (will submit another PR to clean these up). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one little missed spot and one nit.
This allows all the structs and enums to easily be placed in a struct which derives Debug itself.
9f9ab78
to
c16f43b
Compare
Rebased on main. |
Thanks again for going through all the small nits and revisions on this one. 👍🏻 |
No probs. static METRICS: Lazy<Metrics> = Lazy::new(Metrics::new);
#[derive(Debug)]
struct Metrics {
pub clear_region_count: Counter,
pub clear_region_duration: Histogram,
pub draw_count: Counter,
pub draw_duration: Histogram,
pub append_lines_count: Counter,
pub append_lines_duration: Histogram,
pub hide_cursor_duration: Histogram,
pub show_cursor_duration: Histogram,
pub get_cursor_position_duration: Histogram,
pub set_cursor_position_duration: Histogram,
pub size_duration: Histogram,
pub window_size_duration: Histogram,
pub flush_count: Counter,
pub flush_duration: Histogram,
} |
Signed-off-by: Toby Lawrence <[email protected]>
Released as part of Thanks again for your contribution! |
This allows all the structs and enums to easily be placed in a struct
which derives Debug itself.
The metrics::recorder::Recorder and metrics_tracing_context::LabelFilter
traits now also extend Debug.