4 ms·

I agree. Personally, I'd prefer let span = 5.days() + 8.hours() + 1.minutes();

csomar 2y ago
I wonder if OP will accept a PR for such a change. Your proposal is much readable and flexible (it's not clear from the docs if you can add random time ranges together). Plus, you'll be able to create your own ranges like `1.decade` or `1.application_timeframe` and add/subtract them.
mijoharas 2y ago
Have you checked the API to see if that works? I imagine it does.
mutatio 2y ago
Looks like it should be supported: https://docs.rs/jiff/latest/jiff/struct.Span.html#impl-Add%3CSpan%3E-for-%26Zoned https://docs.rs/jiff/latest/jiff/struct.Span.html#impl-Add%3...
- wging 2y ago
  That isn't an implementation of addition between Spans and other Spans. It looks like there isn't one in the library right now. `impl<'a> Add<Span> for &'a Zoned` means a borrow of Zoned is on the left hand side, and a Span on the right. So it says that if z is a Zoned (not a Span) and s is a Span, you can do `&z + s` to add a span to a Zoned. There are a bunch of implementations there, DateTime + Span, Date + Span, Time + Span, Offset + Span. All with Span on the right, but none for Span + Span (nor Span + &Span, or &Span + &Span, ...).
  - burntsushi 2y ago
    This is correct. You can't do a `span1 + span2`. You'd have to use `span1.checked_add(span2)`. The main problem I had with overloading `+` for span addition is that, in order to add two spans with non-uniform units (like years and months), you need a relative datetime. So `+` would effectively have to panic if you did `1.year() + 2.months()`, which seems like a horrific footgun. It would be plausible to make `+` for spans do _only_ component wise addition, but this would be an extremely subtle distinction between `+` and `Span::checked_add`. To the point where sometimes `+` and `checked_add` would agree on the results and sometimes they wouldn't. I think that would also be bad. So I started conservative for the time being: no `+` for adding spans together.
    - stouset 2y ago
      What's the issue with having a span represent "one year and two months"? Both involve a variable number of days (365-366 and 28-31), but I would hope you could store the components separately so it can be unambiguously applied once given a specific moment in time. I'm thinking something along the lines of how ActiveSupport::Duration works: >> 4.years + 5.months + 3.weeks + 2.days + 1.hour + 4.minutes + 10.seconds => 4 years, 5 months, 3 weeks, 2 days, 1 hour, 4 minutes, and 10 seconds Of course the downside being that it would need to be sized large enough to contain each component, even though they may be rarely used.
      - burntsushi 2y ago
        The components are stored separately. I think what you are advocating for is component wise addition, which I mentioned in my previous comment. It can totally "work," but as I said, it will produce different results than `span1.checked_add(span2)`. For example: use jiff::{ToSpan, Unit, Zoned}; fn main() -> anyhow::Result<()> { let span1 = 1.year().months(3); let span2 = 11.months(); let now = Zoned::now().round(Unit::Minute)?; let added = span1.checked_add((span2, &now))?; println!("{added}"); Ok(()) } Has this output: $ cargo -q r P2y2m Notice how the months overflow automatically into years. Days will do the same into months. You need a reference point to do this correctly. For example, just `span1.checked_add(span2)?` would produce an error. In contrast, component wise addition would lead to a span of `1 year 14 months`. Which is a valid `Span`. Jiff is fine with it. But it's different than what `checked_add` does. Having both operations seems too subtle. Also, I don't really think using the `+` operator just to construct spans is that much of a win over what Jiff already has. So that needs to be taken into account as well.
        Tobu 2y ago
        The Into<SpanArithmetic> argument feels a little too magical. Since you are not constrained by a trait, I would prefer something like Span::add_with_reference(&self, other: &Span, reference: impl Into<SpanRelativeTo>)

I wonder if OP will accept a PR for such a change. Your proposal is much readable and flexible (it's not clear from the docs if you can add random time ranges together). Plus, you'll be able to create your own ranges like `1.decade` or `1.application_timeframe` and add/subtract them.

mijoharas 2y ago

Have you checked the API to see if that works? I imagine it does.

mutatio 2y ago

Looks like it should be supported: https://docs.rs/jiff/latest/jiff/struct.Span.html#impl-Add%3CSpan%3E-for-%26Zoned https://docs.rs/jiff/latest/jiff/struct.Span.html#impl-Add%3...

wging 2y ago
That isn't an implementation of addition between Spans and other Spans. It looks like there isn't one in the library right now. `impl<'a> Add<Span> for &'a Zoned` means a borrow of Zoned is on the left hand side, and a Span on the right. So it says that if z is a Zoned (not a Span) and s is a Span, you can do `&z + s` to add a span to a Zoned. There are a bunch of implementations there, DateTime + Span, Date + Span, Time + Span, Offset + Span. All with Span on the right, but none for Span + Span (nor Span + &Span, or &Span + &Span, ...).
- burntsushi 2y ago
  This is correct. You can't do a `span1 + span2`. You'd have to use `span1.checked_add(span2)`. The main problem I had with overloading `+` for span addition is that, in order to add two spans with non-uniform units (like years and months), you need a relative datetime. So `+` would effectively have to panic if you did `1.year() + 2.months()`, which seems like a horrific footgun. It would be plausible to make `+` for spans do _only_ component wise addition, but this would be an extremely subtle distinction between `+` and `Span::checked_add`. To the point where sometimes `+` and `checked_add` would agree on the results and sometimes they wouldn't. I think that would also be bad. So I started conservative for the time being: no `+` for adding spans together.
  - stouset 2y ago
    What's the issue with having a span represent "one year and two months"? Both involve a variable number of days (365-366 and 28-31), but I would hope you could store the components separately so it can be unambiguously applied once given a specific moment in time. I'm thinking something along the lines of how ActiveSupport::Duration works: >> 4.years + 5.months + 3.weeks + 2.days + 1.hour + 4.minutes + 10.seconds => 4 years, 5 months, 3 weeks, 2 days, 1 hour, 4 minutes, and 10 seconds Of course the downside being that it would need to be sized large enough to contain each component, even though they may be rarely used.
    - burntsushi 2y ago
      The components are stored separately. I think what you are advocating for is component wise addition, which I mentioned in my previous comment. It can totally "work," but as I said, it will produce different results than `span1.checked_add(span2)`. For example: use jiff::{ToSpan, Unit, Zoned}; fn main() -> anyhow::Result<()> { let span1 = 1.year().months(3); let span2 = 11.months(); let now = Zoned::now().round(Unit::Minute)?; let added = span1.checked_add((span2, &now))?; println!("{added}"); Ok(()) } Has this output: $ cargo -q r P2y2m Notice how the months overflow automatically into years. Days will do the same into months. You need a reference point to do this correctly. For example, just `span1.checked_add(span2)?` would produce an error. In contrast, component wise addition would lead to a span of `1 year 14 months`. Which is a valid `Span`. Jiff is fine with it. But it's different than what `checked_add` does. Having both operations seems too subtle. Also, I don't really think using the `+` operator just to construct spans is that much of a win over what Jiff already has. So that needs to be taken into account as well.
      - Tobu 2y ago
        The Into<SpanArithmetic> argument feels a little too magical. Since you are not constrained by a trait, I would prefer something like Span::add_with_reference(&self, other: &Span, reference: impl Into<SpanRelativeTo>)