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

Missing method to initialize a DateTime with non-zero millis or nanos #873

Open
Guara92 opened this issue Nov 14, 2022 · 10 comments
Open

Missing method to initialize a DateTime with non-zero millis or nanos #873

Guara92 opened this issue Nov 14, 2022 · 10 comments

Comments

@Guara92
Copy link

Guara92 commented Nov 14, 2022

Trying to upgrade to 0.4.23 I'm facing a limitation of the trait TimeZone where now the only way to get a DateTime is via with_ymd_and_hms that assume nanos always 0.
I think that the trait should expose a method to override this behaviour or ymd_opt shouldn't be deprecated, in particular my usecase is something like:

Utc.ymd_opt(2022, 11, 14).unwrap().and_hms_milli_opt(23, 59, 59, 999).unwrap(),

I'm missing a way to do this?

@djc
Copy link
Member

djc commented Nov 14, 2022

Yes, the recommended way to do this is something like NaiveDateTime::from_ymd_opt(2022, 11, 14).and_then(|d| d.and_hms_milli_opt(23, 59, 59, 999)).unwrap().and_local_timezone(Utc).

(I think the deprecation message was trying to point you in this direction?)

@Guara92
Copy link
Author

Guara92 commented Nov 14, 2022

Thanks your solution is ok but maybe too verbose? there is a specific reason to deprecate ymd_opt?
(the deprecation message is saying use "with_ymd_and_hms()" instead)

@esheppa
Copy link
Collaborator

esheppa commented Nov 14, 2022

there is a specific reason to deprecate ymd_opt?

ymd_opt was a method that produced a Date<Tz> which has also been deprecated.

NaiveDateTime::from_ymd_opt(2022, 11, 14)?.and_hms_milli_opt(23, 59, 59, 999)?.and_local_timezone(Utc)?

is only slightly more verbose than

Utc.ymd_opt(2022, 11, 14)?.and_hms_milli_opt(23, 59, 59, 999)?

but has an added advantage that you don't need to import the TimeZone trait anymore.

However I'm keen to understand occurrence of usages like this in code bases to see where we can possibly improve the API.

From my own experience in codebases, I'm thinking of the following:

  • and_time is infallible so having a NaiveTime::START or NaiveTime::MIN would be useful to combine with this, or perhaps even a NaiveDate::start_time method. NaiveTime::END is problematic because of leap seconds, but the start seems more generally useful
  • I often want the first or last day in a given year or month, some thoughts here are NaiveDate::start_of_year(year: i32), NaiveDate::start_of_month(year: i32, month: Month) which may be able to be implemented infallibly.

Would these and/or similar methods assist in reducing the total amount of verbosity?

@djc
Copy link
Member

djc commented Nov 14, 2022

(We wouldn't want to implement start_of_year(year: i32, month: Month) infallibly because the year range could still be off.)

@esheppa
Copy link
Collaborator

esheppa commented Nov 15, 2022

I'm trying out some ideas extending #845 where the year part of the date is infallible, but represented by a i16. IMO reducing the year range, but having it be infallible could be an excellent tradeoff if it opens up the possibilities mentioned above

@mathstuf
Copy link
Contributor

I think this would be a lot easier if something like Utc.with_ymd_and_hms(…).unwrap().add_millis_opt(…).unwrap() were possible.

@djc
Copy link
Member

djc commented Nov 16, 2022

I'm trying out some ideas extending #845 where the year part of the date is infallible, but represented by a i16. IMO reducing the year range, but having it be infallible could be an excellent tradeoff if it opens up the possibilities mentioned above

Yeah, that tradeoff definitely makes a lot of sense!

I think this would be a lot easier if something like Utc.with_ymd_and_hms(…).unwrap().add_millis_opt(…).unwrap() were possible.

Anything that has more than one unwrap() is suboptimal, of course. Would like to find other ways of doing it.

@eilidh-spire
Copy link

Came up against this recently, and there isn't really a pretty way around it. In the end I used Utc.with_ymd_and_hms_opt(...).unwrap() + Duration::milliseconds(...) 🙃

Would be really nice to be able to just specify all components at once. How about somthing like Utc.with_ymd_and_hms_and_millis_opt(...).unwrap()? Too unwieldy?

@esheppa
Copy link
Collaborator

esheppa commented Dec 2, 2022

@eilidh-spire do any of the solutions proposed in #815 look ok? These could be extended as you describe with a milliseconds parameter as well

@eilidh-spire
Copy link

do any of the solutions proposed in #815 look ok?

@esheppa thanks for linking, I'm all for fallible constructors being the default - much better to have the compiler make us handle erroneous input, and even if we just unwrap at the end of the day, it at least makes the possible panic explicit.

We're mostly using these functions to construct UTC dates in test code, so the main concern is really being succinct and readable - a li'l .unwrap() probably isn't going to ruin anyone's day when reading/writing a test. Const compile-time validation does seems really neat for the occasional situation where it could be applied in production code though.

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

5 participants