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

Timestamp::saturating_add misbehaves if the span contains calendar units #36

Open
FeldrinH opened this issue Jul 23, 2024 · 1 comment
Labels
breaking change Issues that require a breaking change for resolution. bug Something isn't working

Comments

@FeldrinH
Copy link

let now = Timestamp::now();
let span = 1.day();
println!("{}", now.saturating_add(span));

Expectation: Error, panic or some other indication that addition failed.

Actual result: 9999-12-30T22:00:00.999999999Z

@BurntSushi BurntSushi added the bug Something isn't working label Jul 23, 2024
@BurntSushi
Copy link
Owner

Thank you for catching this. This was a bad oversight on my part. I think I had written the saturating method routine before I smoothed out the error cases for checked arithmetic. This will indeed mean that Timestamp::saturating_add should return an error. I might make it panic in the interim before a jiff 0.2 release.

This also motivates #21, because with an "absolute" duration, we can provide infallible saturating arithmetic on Timestamp.

@BurntSushi BurntSushi added the breaking change Issues that require a breaking change for resolution. label Jul 23, 2024
BurntSushi added a commit that referenced this issue Jul 26, 2024
The saturating methods *should* return an error. But I goofed on the
API. So at this point, our two choices are: leave it as-is, which
produces completely incorrect results when there are non-zero units of
days or greater, or panic when it occurs. I think a panic is better.

In Jiff 0.2, these APIs will be made fallible. And hopefully by then
we'll have truly infallible APIs that accept an "absolute" duration.

Partially addresses #36
BurntSushi added a commit that referenced this issue Jul 26, 2024
The saturating methods *should* return an error. But I goofed on the
API. So at this point, our two choices are: leave it as-is, which
produces completely incorrect results when there are non-zero units of
days or greater, or panic when it occurs. I think a panic is better.

In Jiff 0.2, these APIs will be made fallible. And hopefully by then
we'll have truly infallible APIs that accept an "absolute" duration.

Partially addresses #36
BurntSushi added a commit that referenced this issue Jul 26, 2024
The saturating methods *should* return an error. But I goofed on the
API. So at this point, our two choices are: leave it as-is, which
produces completely incorrect results when there are non-zero units of
days or greater, or panic when it occurs. I think a panic is better.

In Jiff 0.2, these APIs will be made fallible. And hopefully by then
we'll have truly infallible APIs that accept an "absolute" duration.

Partially addresses #36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Issues that require a breaking change for resolution. bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants