Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add an option to specify a unit for the zero value in the SpanPrinter #192

Closed
azerupi opened this issue Jan 2, 2025 · 4 comments · Fixed by #194
Closed

Add an option to specify a unit for the zero value in the SpanPrinter #192

azerupi opened this issue Jan 2, 2025 · 4 comments · Fixed by #194
Labels
enhancement New feature or request

Comments

@azerupi
Copy link

azerupi commented Jan 2, 2025

For my use case I'm pretty printing spans rounded up to the minute. However when a zero span is printed it shows up in seconds instead and the only way that I see to influence that is to mark a unit as fractional.

In an ideal scenario a rounded span could propagate its lower limit to the printer to avoid having to repeat the lower bound multiple times. But I understand that this would require to store the information in the span to handle the zero case, which is probably not desirable.

There could be a method to the printer to specify the zero value unit. But this could potentially lead to mismatches between the rounded span's lowest unit and the zero value unit and it would also require the user to specify the lowest unit twice, once for the rounding and once for the printer.

Maybe a method could be added to the printer to apply the rounding to the span and print it. That would allow the zero value to be printed in the same unit as the rounding.

What are your thoughts on this? Are there better solutions for this use case?

@BurntSushi
Copy link
Owner

I definitely don't want to be coupling rounding with the printer. It was very intentional that rounding is kept separate from the printer. And especially since in a lot of cases, rounding is done as part of datetime arithmetic anyway, e.g., Zoned::until (which is introducing its own form of coupling borrowed from the Temporal API design).

And yeah, attaching more metadata to Span is a no-no I think. I suspect it wouldn't compose well. And it increases the size of a type that is already huge.

If that coupling is off the table, then I don't think I see a way to do this without repeating information. Adding a knob to the printer is easy enough to do I think. I would be fine with that. But yeah, it does require some careful attention to the zero span to get this right.

Another option aside from a printer knob is to just special case it yourself. The friendly format is really easy to format to in an ad hoc way (unless you're writing a highly configurable printer like that found in Jiff, lol):

let span = <insert rounding code>;
if span.is_zero() {
    write!(f, "0m")
} else {
    write!(f, "{span:#}")
}

@BurntSushi BurntSushi added the enhancement New feature or request label Jan 2, 2025
@azerupi
Copy link
Author

azerupi commented Jan 3, 2025

Another option aside from a printer knob is to just special case it yourself.

That's fair 🙂 It would remove a little bit of friction if jiff could do it for us, but special casing it on the user side is easy enough. Thanks!

BurntSushi added a commit that referenced this issue Jan 4, 2025
This adds a new option for configuring the unit to use when formatting a
zero span or duration.

Closes #192
BurntSushi added a commit that referenced this issue Jan 4, 2025
This adds a new option for configuring the unit to use when formatting a
zero span or duration.

Closes #192
BurntSushi added a commit that referenced this issue Jan 4, 2025
This adds a new option for configuring the unit to use when formatting a
zero span or duration.

Closes #192
@BurntSushi
Copy link
Owner

This should be available on crates.io in jiff 0.1.21.

@azerupi
Copy link
Author

azerupi commented Jan 4, 2025

Great, thank you!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants