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

Ergonomics for naive::Days and Months #1208

Open
nathan opened this issue Jul 28, 2023 · 6 comments
Open

Ergonomics for naive::Days and Months #1208

nathan opened this issue Jul 28, 2023 · 6 comments

Comments

@nathan
Copy link

nathan commented Jul 28, 2023

naive::Days and Months are extremely unergonomic at the moment; it's impossible to recover the number of days/months once one has been constructed, and the constructors Days::new and Months::new are rather verbose.

Would it be possible for chrono to provide From and Into impls for u64/u32 (respectively) and/or make each integer pub so users can construct and destructure them?

@djc
Copy link
Contributor

djc commented Jul 29, 2023

What's your use case for this? It seems unergonomic because you're using them in a way that we didn't foresee when designing them.

Also how is Days::new() more verbose than Days::from()? I suppose you want to use .into() instead?

@nathan
Copy link
Author

nathan commented Jul 30, 2023

I suppose you want to use .into() instead?

Indeed, or the constructor Days().

What's your use case for this?

E.g., if I want an iterator that yields the same day/time on consecutive months, I currently write:

struct It {
    base: DateTime<Utc>,
    here: u32,
}
impl It {
    fn new(base: DateTime<Utc>) -> Self {
        Self { base, here: 0 }
    }
}
impl Iterator for It {
    type Item = DateTime<Utc>;
    fn next(&mut self) -> Option<Self::Item> {
        let m = self.here;
        self.here += 1;
        self.base.checked_add_months(Months::new(m))
    }
}

fn main() {
    let dt = Utc.with_ymd_and_hms(2023, 07, 31, 12, 34, 56).single().unwrap();
    dbg!(It::new(dt).take(5).collect::<Vec<_>>());
}

Whereas the following would be easier and clearer (less unit confusion):

// ...
    here: Months,
// ...
        Self { base, here: Months(0) }
// ...
        let m = self.here;
        self.here += Months(1); // or self.here.0 += 1, vel sim.
        self.base.checked_add_months(m)

I also can't write a helper that accepts Months and does anything but pass it on unmodified:

fn consecutive(dt: DateTime<Utc>, months: Months) -> Vec<DateTime<Utc>> {
    It::new(dt).take(months/* uh oh... */).collect()
}

When I initially encountered Days and Months I assumed they were newtypes meant to help manage units, but you can't actually use them like that. If the only purpose of these types is to force you to write Months::new() in every call to very-explicitly-named methods like checked_add_months, I don't see why they exist at all (the Add/Sub impls, I suppose?).

@djc
Copy link
Contributor

djc commented Aug 1, 2023

Yes, they mostly exist for the Add/Sub impls. I don't want to expose the inner field publicly so you won't be able to write Days(1) directly.

@nathan
Copy link
Author

nathan commented Aug 2, 2023

I don't want to

Ok…

@danwilliams
Copy link
Contributor

@nathan I don't really follow about the constructors, but regarding From, this PR adds Months::as_u32(), which should allow you to determine the value of months. Using that method you can then easily implement any From conversions you need 🙂

@djc
Copy link
Contributor

djc commented Jan 10, 2024

Also I think all of this will be superseded by the CalendarDuration type as proposed in #1290.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants