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

Nicer (alternative?) Debug implementations #503

Closed
aldanor opened this issue Aug 27, 2022 · 7 comments · Fixed by #509
Closed

Nicer (alternative?) Debug implementations #503

aldanor opened this issue Aug 27, 2022 · 7 comments · Fixed by #509
Labels
A-core Area: anything not otherwise covered C-enhancement Category: an enhancement with existing code

Comments

@aldanor
Copy link

aldanor commented Aug 27, 2022

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 🖖

@jhpratt jhpratt added A-core Area: anything not otherwise covered C-enhancement Category: an enhancement with existing code labels Aug 27, 2022
@jhpratt
Copy link
Member

jhpratt commented Aug 27, 2022

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.

@xy2i
Copy link
Contributor

xy2i commented Sep 24, 2022

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.

@jhpratt
Copy link
Member

jhpratt commented Sep 25, 2022

@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.

@xy2i
Copy link
Contributor

xy2i commented Sep 25, 2022

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: [] }] } }

@aldanor
Copy link
Author

aldanor commented Sep 25, 2022

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
Copy link
Author

aldanor commented Sep 25, 2022

@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.

@jhpratt
Copy link
Member

jhpratt commented Sep 25, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: anything not otherwise covered C-enhancement Category: an enhancement with existing code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants