time icon indicating copy to clipboard operation
time copied to clipboard

Nicer (alternative?) `Debug` implementations

Open aldanor opened this issue 2 years ago • 1 comments

Here's one simple suggestion that may increase user-friendliness of the crate by a huge margin:

  • Switch Debug implementations (e.g. for Date or OffsetDateTime) to user-friendly ones

Current behaviour if you ever debug-print a date or a datetime (you'll have to scroll right...):

date: Date { year: 2019, ordinal: 89 },
time: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2019, ordinal: 89 }, time: Time { hour: 0, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } } },

Proposed behaviour:

date: 2019-03-30,
time: 2019-03-30 0:00:00.0 +00:00:00,

The main point, really, is user (or rather, dev) friendliness, to aid in developing crates that depend on time. As a hypothetical example, you're using this crate for parsing some structs from some APIs and you want to quickly derive Debug on those to see what you've just parsed, you'll have a hard time (especially with dates).

A few notes and some explanation:

  • Whichever formatting is chosen, it has to be lossless, especially for datetimes (that is, if there are non-zero nanoseconds there, they cannot be truncated).
  • Date { year: 2019, ordinal: 89 } is information for computers, not for human beings. You won't be able to tell what date it is without doing a mental (or computer-aided) computation first.
  • OffsetDateTimes are absolutely humongous when debug-printed and will likely dwarf most structs; their readable representation is a tiny string that humans can immediately parse though.
  • But Debug has to represent internals since it's meant for Debug purposes? Not really - same as Vec's Debug implementation doesn't print its capacity and the pointer, it just outputs whatever likely is the most useful information to the user. If you need to print capacity and the pointer, in very rare cases, you can always do so manually.
  • Why not implement your own Debug? You can. But sometimes you have 20 other fields in your structs, and you'd have to manually implement Debug just to be able to see your dates and times in a readable fashion.
  • You can always wrap Date in a newtype? You can. But then have to do it everywhere you use it, you'll have to implement derefs and conversions and possible end up with a bunch of newtype wrappers, and all for the sake of being able to just print it?
  • But it already implements Display? It does. But more often than not your structs and their fields won't, so you won't be able to derive it even with helper crates like derive_more.
  • Performance-wise, it doesn't really matter. It's pretty-formatting, after all.
  • If the current debug-representation is really needed, one could add a newtype wrapper for each struct which will be accessible as e.g. struct.debug(). The chances are, it will never get used, except by the devs of this crate themselves. An alternative option, if needed for dev purposes, is having a feature flag, disabled by default, for auto-derived debug (current behaviour); but then again, would it be used at all?

I could try and help out with this if needed (although it should be pretty trivial implementation-wise).

Thanks and cheers 🖖

aldanor avatar Aug 27 '22 00:08 aldanor

Thank you for the detailed and thorough issue!

I'll think about it. Ultimately a debug representation is meant for debugging, and this includes debugging the time crate. Some of the debug outputs are not even representative of the actual internal representation, such as Date (Date is internally stored as a bitpacked u32).

With regard to the comparison to Vec, it also matters how the value is semantically thought about. Aside from OffsetDateTime (which uses PrimitiveDateTime internally), this is pretty much already the case. For what it's worth OffsetDateTime will have a different output come October, as that's when the internals for OffsetDateTime and PrimitiveDateTime will be merged.

Date could be changed to be a year-month-day debug output, but that would have some cost associated with it (namely calculating the month and day). That's not the end of the world, as the debug output shouldn't be used in places where performance matters anyways.

jhpratt avatar Aug 27 '22 05:08 jhpratt

Hey, ran into the same problem. Another inconvenience is that Rust tests output Debug print by default, so our tests look like this when they fail: Screenshot_20220924_134134

I went ahead and made PR #509, which copy the Display impls that are the same as you describe in the issue.

xy2i avatar Sep 24 '22 18:09 xy2i

@xy2iii Would you mind sharing that as text? It's a decent case for me to visualize how different changes would affect the output, even if this one is on the larger-than-average end of the spectrum.

jhpratt avatar Sep 25 '22 03:09 jhpratt

Here are the cases of #509 on this struct:

Before

Resource { id: 0, meta: Meta { object: Report, url: "https://api.wanikani.com/v2/summary", data_updated_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 11, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } } }, data: SummaryR { lessons: [AvailableLessons { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 11, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [7746, 208, 204] }], next_reviews_at: Some(OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 11, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }), reviews: [AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 11, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [776, 2937] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 12, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 13, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [7690, 3248] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 14, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 15, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [3485, 4164] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 16, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [3483] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 17, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [8939, 7745, 2778, 7687, 7689, 3310, 8801] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 18, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 19, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 20, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 21, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 22, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 267 }, time: Time { hour: 23, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 268 }, time: Time { hour: 0, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 268 }, time: Time { hour: 1, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 268 }, time: Time { hour: 2, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [3270, 2595] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 268 }, time: Time { hour: 3, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [7629, 803, 3224, 3136, 3124, 2878, 3243, 7478] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 268 }, time: Time { hour: 4, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 268 }, time: Time { hour: 5, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 268 }, time: Time { hour: 6, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 268 }, time: Time { hour: 7, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [8665, 3249, 7535, 3207, 7625] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 268 }, time: Time { hour: 8, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [3278, 3271] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 268 }, time: Time { hour: 9, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [4848] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 268 }, time: Time { hour: 10, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }, AvailableReviews { available_at: OffsetDateTime { local_datetime: PrimitiveDateTime { date: Date { year: 2022, ordinal: 268 }, time: Time { hour: 11, minute: 0, second: 0, nanosecond: 0 } }, offset: UtcOffset { hours: 0, minutes: 0, seconds: 0 } }, subject_ids: [] }] } }

After

Resource { id: 0, meta: Meta { object: Report, url: "https://api.wanikani.com/v2/summary", data_updated_at: 2022-09-24 11:00:00.0 +00:00:00 }, data: SummaryR { lessons: [AvailableLessons { available_at: 2022-09-24 11:00:00.0 +00:00:00, subject_ids: [7746, 208, 204] }], next_reviews_at: Some(2022-09-24 11:00:00.0 +00:00:00), reviews: [AvailableReviews { available_at: 2022-09-24 11:00:00.0 +00:00:00, subject_ids: [776, 2937] }, AvailableReviews { available_at: 2022-09-24 12:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-24 13:00:00.0 +00:00:00, subject_ids: [7690, 3248] }, AvailableReviews { available_at: 2022-09-24 14:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-24 15:00:00.0 +00:00:00, subject_ids: [3485, 4164] }, AvailableReviews { available_at: 2022-09-24 16:00:00.0 +00:00:00, subject_ids: [3483] }, AvailableReviews { available_at: 2022-09-24 17:00:00.0 +00:00:00, subject_ids: [8939, 7745, 2778, 7687, 7689, 3310, 8801] }, AvailableReviews { available_at: 2022-09-24 18:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-24 19:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-24 20:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-24 21:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-24 22:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-24 23:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-25 0:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-25 1:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-25 2:00:00.0 +00:00:00, subject_ids: [3270, 2595] }, AvailableReviews { available_at: 2022-09-25 3:00:00.0 +00:00:00, subject_ids: [7629, 803, 3224, 3136, 3124, 2878, 3243, 7478] }, AvailableReviews { available_at: 2022-09-25 4:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-25 5:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-25 6:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-25 7:00:00.0 +00:00:00, subject_ids: [8665, 3249, 7535, 3207, 7625] }, AvailableReviews { available_at: 2022-09-25 8:00:00.0 +00:00:00, subject_ids: [3278, 3271] }, AvailableReviews { available_at: 2022-09-25 9:00:00.0 +00:00:00, subject_ids: [4848] }, AvailableReviews { available_at: 2022-09-25 10:00:00.0 +00:00:00, subject_ids: [] }, AvailableReviews { available_at: 2022-09-25 11:00:00.0 +00:00:00, subject_ids: [] }] } }

xy2i avatar Sep 25 '22 06:09 xy2i

Just to chime in, one of my use cases was somewhat similar to @xy2iii's.

If during testing any of the asserts failed, you'd be left with screens worth of ordinals, offsets and nanoseconds if your types had #[derive(Debug)] and contained time types within them.

aldanor avatar Sep 25 '22 09:09 aldanor

@jhpratt Just a thought on this:

this includes debugging the time crate

As a middleground solution, why couldn't a feature be added (disabled by default), like "debug-internal-repr", in which case all debug implementation revert back to dumping all of their internals. It's not perfect, but provides some flexibility. This way, time devs can trace errors if they occur, while at the same time the users are happy because they would almost never care about the internal representation when deriving Debug.

aldanor avatar Sep 25 '22 09:09 aldanor

Alright…there's 28 instances of an OffsetDateTime being output there. Needless to say that's a lot. However, I can't say that having an OffsetDateTime in a Vec is an unusual situation.

I'll think about this some more and should make a decision within the coming days. One thing I will not be doing is adding a feature flag for this — that just makes it more complicated to maintain, increases CI time, and would likely be confusing to downstream users.

jhpratt avatar Sep 25 '22 22:09 jhpratt