Hacker News Reader

Comment by frereit | Hacker News Reader

I agree. Personally, I'd prefer

    let span = 5.days() + 8.hours() + 1.minutes();

View on HN

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

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, ...).

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.

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.

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.

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.

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

Reply on news.ycombinator.com