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

Span: Ergonomics Improvements #40

Closed
martintmk opened this issue Jul 24, 2024 · 2 comments · Fixed by #47
Closed

Span: Ergonomics Improvements #40

martintmk opened this issue Jul 24, 2024 · 2 comments · Fixed by #47
Labels
enhancement New feature or request

Comments

@martintmk
Copy link

First, let me say that I love the library. It just makes sense and the APIs are great to work with. Here are my thoughts after playing with the Span type:

Easy conversion into the Duration type:

It's a very common for service to get a span by subtracting timestamps. For example, by parsing retry-after header and applying sleep operation. Usually, sleep operation works with Duration and ti would be nice to have a built-in support for converting Span into Duration:

let span = 1.minute();
let duration: Duration = span.try_into().unwrap();
assert_eq!(duration, Duration::from_secs(60));

Check whether the Span is positive

Currently, there is is_zero and is_negative APIs on the Span, I miss the is_positive method that tells me the Span contains som non-zero positive value. Usage would be:

let diff = timestamp_2 - timestamp_1;

if diff.is_positive() {
    thread::sleep(diff.try_into());
} 

I was also missing saturating_add and saturating_sub, but that's covered by #10.

@BurntSushi
Copy link
Owner

BurntSushi commented Jul 24, 2024

For Duration, I think that's #21. Although it's important to note that spans with units greater than days require a reference date. So you won't be able to just call Duration::try_from(span) because it doesn't have enough information. With that said, we could provide a TryFrom<Span> for Duration that just fails if it has any units above days. I'm not 100% certain that's an OK API to add, but I think it probably seems fine.

One approach that can be done today is to feed the output of Span::total into Duration::try_from_secs_f64. That will work even with calendar units since Span::total requires a reference date for spans with non-zero units greater than days.

Now, going from Duration to Span is much easier, or would be easier if Duration were signed. Otherwise it would just be Span::new().seconds(d.as_secs()).nanoseconds(d.subsec_nanos()). So a TryFrom<Duration> for Span would be very nice to have. (It could almost be infallible except for the fact that a Duration could result in overflow of a Span.)

I'm definitely fine with adding is_positive.

@BurntSushi BurntSushi added the enhancement New feature or request label Jul 25, 2024
BurntSushi added a commit that referenced this issue Jul 26, 2024
This PR dips our toes into the water of integration with
`std::time::Duration`. I do somewhat expect there to be more integration
points, particularly in the datetime arithmetic APIs, but this at least
paves a path for folks to _correctly_ convert between a `Duration` and a
`Span`. The error cases for the new APIs will ensure you can never
misuse these APIs.

This adds a few new APIs:

* `Span::is_positive`, since we already have `Span::is_negative` and
  `Span::is_zero`.
* `Span::signum`, since it's signed and it's sometimes convenient.
* `TryFrom<std::time::Duration> for Span`.
* `TryFrom<Span> for std::time::Duration` that returns an error if
  the span is negative or has non-zero units bigger than days.
* `Span::to_duration` that requires a relative date and can convert any
  positive span into a `Duration` (modulo overflow).

Partially addresses #21

Fixes #40
@BurntSushi
Copy link
Owner

See: #21 (comment)

# 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