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

Timestamp adds 1 day panic #90

Closed
tisonkun opened this issue Aug 11, 2024 · 3 comments · Fixed by #91
Closed

Timestamp adds 1 day panic #90

tisonkun opened this issue Aug 11, 2024 · 3 comments · Fixed by #91
Labels
enhancement New feature or request

Comments

@tisonkun
Copy link

tisonkun commented Aug 11, 2024

let current_date = Timestamp::now();
current_date + 1.day()

panic:

adding span to timestamp overflowed: Error { inner: ErrorInner { kind: Range(RangeError(Negative { what: "days", given: 1, min: -7304484, max: 7304484 })), cause: None } }
thread 'append::rolling_file::rotation::tests::test_next_date_timestamp' panicked at /Users/tison/.cargo/registry/src/index.crates.io-6f17d22bba15001f/jiff-0.1.5/src/timestamp.rs:2233:31:
adding span to timestamp overflowed: Error { inner: ErrorInner { kind: Range(RangeError(Negative { what: "days", given: 1, min: -7304484, max: 7304484 })), cause: None } }
stack backtrace:

Code:

// Span
    #[inline(always)]
    pub(crate) fn smallest_non_time_non_zero_unit_error(
        &self,
    ) -> Option<Error> {
        if self.days != 0 {
            Some(t::SpanDays::error("days", self.days.get()))
        } else if self.weeks != 0 {
            Some(t::SpanWeeks::error("weeks", self.weeks.get()))
        } else if self.months != 0 {
            Some(t::SpanMonths::error("months", self.months.get()))
        } else if self.years != 0 {
            Some(t::SpanYears::error("years", self.years.get()))
        } else {
            None
        }
    }
@tisonkun
Copy link
Author

tisonkun commented Aug 11, 2024

Not sure why we have this check ..

@BurntSushi
Copy link
Owner

The panic is occurring here because you're using the + operator, which specifically panics when checked_add fails. So if you want to deal with error conditions gracefully, then you should use checked_add. And checked_add documents this error condition. You can't add days or greater units to a Timestamp because the meaning of "day" is ambiguous in this context.

@BurntSushi BurntSushi added the enhancement New feature or request label Aug 11, 2024
@BurntSushi
Copy link
Owner

The error message here is pretty abysmal though. So we should improve that.

BurntSushi added a commit that referenced this issue Aug 11, 2024
Previously, we would report a range error when trying to use units of
days or greater in a span when adding to a `Timestamp`. But this error
doesn't really do a good job of explaining anything. It was a hold-over
from the days where I was using many different error types and it was
difficult to improve messages on an ad hoc basis. But now, we can just
write a message.

The error message doesn't explain *why* days-and-greater are banned, but
it's hard to fit all of that into an error. At some point, we just have
to rely on folks reading the docs.

Closes #90
BurntSushi added a commit that referenced this issue Aug 11, 2024
Previously, we would report a range error when trying to use units of
days or greater in a span when adding to a `Timestamp`. But this error
doesn't really do a good job of explaining anything. It was a hold-over
from the days where I was using many different error types and it was
difficult to improve messages on an ad hoc basis. But now, we can just
write a message.

The error message doesn't explain *why* days-and-greater are banned, but
it's hard to fit all of that into an error. At some point, we just have
to rely on folks reading the docs.

Closes #90
# 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