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

Ergonomic replacements for panicking APIs. #1491

Open
turalcar opened this issue Mar 6, 2024 · 11 comments
Open

Ergonomic replacements for panicking APIs. #1491

turalcar opened this issue Mar 6, 2024 · 11 comments

Comments

@turalcar
Copy link

turalcar commented Mar 6, 2024

When a lot of panicking APIs, such as NaiveDate::from_ymd() were deprecated I thought it to be a momentary aberration and locked the version at 0.4.22. A year has passed and proven me wrong.
I replaced .pred().pred() with - Duration::days(2) and now that's deprecated too.
In all of my code from_ymd(), from_hms() etc were only used ever with literals. I don't think that's uncommon. One could try to enforce that with a macro (e. g. date!(2024, 3, 6) similarly to how print!("Hi") only accepts string literals).
Whatever a good replacement is, it should've been implemented before the old version was deprecated.

@pitdicker
Copy link
Collaborator

pitdicker commented Mar 6, 2024

@turalcar Sorry for all the deprecations. I understand they cause churn and make chrono less nice to use!

The plan is to move chrono away from a panic-by-default design to better match current rust standards.
The intention is not to reverse the deprecations.

Maybe the macro's I proposed in #1270 can be a help to you? I certainly look forward to them a lot for the same reason.
It should not be hard to port them to your project(s).

Also most methods are now const. Maybe that allows you to move some of the fallibility to compile-time?

@Lorak-mmk
Copy link

Moving away from panicking APIs is fine, but there should be more time between introducing new APIs and deprecating old.
Current situation forces us to move to new APIs - which is fine - and significantly bump our chrono dependency minimum version and I'm reluctant to impose this on our users.

@pitdicker
Copy link
Collaborator

and significantly bump our chrono dependency minimum version and I'm reluctant to impose this on our users.

May I ask what your thoughts are WRT having a minimum chrono dependency version? Are you 'reluctant to impose this' because of the deprecation warnings? And what would you consider a reasonable time?

@Lorak-mmk
Copy link

My thinking is that some of our users may not be so fast to update their dependencies. If they stay on older chrono versions, and we require a new one, then they have to either

  1. stay on older version of our library which uses older chrono
  2. be forced to update to newer version of chrono quicker
  3. Use newer chrono versions, despite not properly migrating to it - so probably needing to disable deprecation warning in the project.

Scenario 2 is good - more apps using more recent versions of library, yay. Other scenarios not so much :<
From a purely selfish perspective I prefer major releases to deprecations - in that case we can support both versions, with support guarded by feature flags, and new releases don't break CI :D
But I understand that it's most likely a worse tradeoff for many people, so my comments should probably just be ignored.

Regarding the reasonable time - I'm not sure, I didn't think about it. I just felt that in this case it was a bit too short - deprecation of old APIs a little bit over a month after introduction of new APIs, and after 2 minor releases (from what I see: 25 January, 0.4.33 - introduction, today, 0.4.35 - deprecation).

@pitdicker
Copy link
Collaborator

Thank you for the thoughtful comment.

Let me link #1408.

@turalcar
Copy link
Author

turalcar commented Mar 6, 2024

Maybe the macro's I proposed in #1270 can be a help to you?

This looks perfect. I suppose macros for TimeDelta would also be in order.

Also most methods are now const. Maybe that allows you to move some of the fallibility to compile-time?

Any compiler worth its salt would replace the whole NaiveDate::from_ymd_opt(2024, 3, 6).unwrap().with_hms_opt(7, 0, 0).unwrap().and_utc() with a constant so I'll probably leave it as is. Besides, Option::unwrap() isn't const on stable so I'd have to reimplement that too.

@djc
Copy link
Contributor

djc commented Mar 6, 2024

My thinking is that some of our users may not be so fast to update their dependencies. If they stay on older chrono versions, and we require a new one, then they have to either

  1. stay on older version of our library which uses older chrono

  2. be forced to update to newer version of chrono quicker

  3. Use newer chrono versions, despite not properly migrating to it - so probably needing to disable deprecation warning in the project.

I don't see how this could happen. Within a semver-compatible version range, only one copy of a library can be used. So if your downstream users adopt a new version of your crate and that crate requires a newer version of chrono, downstream users will automatically also get the same newer version of chrono.

Do you have a concrete example of any issues like this that have come up?

@Pascualex
Copy link

What is the currently recommended way of initializing a const Duration? Panicking APIs have been deprecated, but unwrap isn't const yet.

@pitdicker
Copy link
Collaborator

pitdicker commented Mar 11, 2024

What is the currently recommended way of initializing a const Duration? Panicking APIs have been deprecated, but unwrap isn't const yet.

I'm also missing a const alternative to unwrap. In chrono we are internally using an expect macro: https://github.com/chronotope/chrono/blob/v0.4.35/src/lib.rs#L718. An unwrap macro can look similar.

Edit:

macro_rules! unwrap {
    ($e:expr) => {
        match $e {
            Some(v) => v,
            None => panic!(),
        }
    };
}

@archer884
Copy link

Yeah, I've never called any of these methods based on anything that WASN'T a constant value. Now I get to go in and add a lot of unwrap calls for the sake of ergonomics. Talk about annoying.

@pitdicker
Copy link
Collaborator

Published chrono 0.4.36 which undoes the deprecations on TimeDelta.

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

6 participants