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

Debug impls should not use Printer implementations #60

Open
addisoncrump opened this issue Jul 30, 2024 · 6 comments
Open

Debug impls should not use Printer implementations #60

addisoncrump opened this issue Jul 30, 2024 · 6 comments
Labels
question Further information is requested

Comments

@addisoncrump
Copy link

addisoncrump commented Jul 30, 2024

See #59 for an example; the printer is opinionated and produces sometimes unrepresentative output. Debug implementations should represent exactly what is stored in memory for testing purposes.

@BurntSushi
Copy link
Owner

Unfortunately, the Debug impls probably need to stay as they are. I share your concern though because, for example, two distinct Span values can have the same Debug representation:

    let span1 = Span::new().seconds(1).nanoseconds(1_001);
    let span2 = Span::new().seconds(1).microseconds(1).nanoseconds(1);
    assert_eq!(format!("{span1:?}"), format!("{span2:?}"));

I mean, both spans represent the same amount of time, but their unit values are distinct. The problem is that the ISO 8601 collapses units smaller than seconds down into "fractional" seconds instead of representing them as distinct units.

I think Span is probably the most egregious offender here. The datetime types I believe shouldn't have this issue.

With that said, there is no specific requirement that a Span's Debug impl use ISO 8601 as a format. We could use a more human friendly representation that doesn't betray the unit values.

The reason why Jiff doesn't just show the internal struct as would be typical is because it's unwieldy and very difficult to read, especially since most units in most spans are zero. That's what I started with. While it's nicer for programmers working on Jiff, it's not nicer for users of Jiff.

@BurntSushi BurntSushi added the question Further information is requested label Jul 30, 2024
@addisoncrump
Copy link
Author

I think the question comes down to "Who will use the Debug implementations?". From my perspective, the Debug impls should be used strictly for debug purposes, i.e. if you want to know the actual content of the data for debug purposes. This is especially relevant due to what you've pointed out: the debug information currently does not provide information that would help someone debug.

It may be appropriate to implement debug s.t. values set to zero are simply omitted and a finish_non_exhaustive is used. But ultimately, hiding the units will make things harder to debug, especially since the units are the user-facing API.

@BurntSushi
Copy link
Owner

I don't think it's quite that clear cut. While hiding units can make things harder to debug, showing too much information can also make things harder to debug. I've spent a lot of time debugging Jiff, and I can say with some confidence that showing all units (even when they are zero) makes things incredibly noisy. The output is easily one or two orders magnitude bigger than is necessary in most cases.

I think the most likely path here is to use something other than ISO 8601 as the output format. But I would still want something that is information dense.

@addisoncrump
Copy link
Author

Hm, okay. Would you consider adding a way to format it with units shown, e.g. with a new type pattern?

@BurntSushi
Copy link
Owner

Even units that are zero? I'd rather not personally. I mean I'm open to it if it becomes a widely requested feature with a solid use case. But for now, I think that can be implemented outside the crate inside the fuzzer if necessary though.

@addisoncrump
Copy link
Author

Ah, not including units that are zero. I'll add it to the shim module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants