time-rs / time

The most used Rust library for date and time handling.
https://time-rs.github.io
Apache License 2.0
1.09k stars 273 forks source link

API improvement: use `impl Into` as function arguments #532

Closed MarcoIeni closed 1 year ago

MarcoIeni commented 1 year ago

Hi 👋 I have a suggestion to simplify creating a Duration.

Current situation

The methods Duration::seconds, Duration::minutes, etc., take an i64 as an argument.

For example, the following works:

    let a = 1;
    let time = time::Duration::seconds(a);

However, when you have other types, you need to explicitly convert the number to an i64 by calling .into():

    let a: u32 = 1;
    let time = time::Duration::seconds(a.into());

Suggestion

Can we change those methods to automatically convert to an i64? For example, Duration::seconds from this:

    pub const fn seconds(seconds: i64) -> Self {
        Self::new_unchecked(seconds, 0)
    }

can become:

    pub const fn seconds(seconds: impl Into<i64>) -> Self {
        Self::new_unchecked(seconds.into(), 0)
    }

The advantage is that the caller doesn't have to write .into() explicitly.

What do you think? I'm not sure if what I'm suggesting is more idiomatic, or if it's better that the caller explicitly calls into(). 🤔

jhpratt commented 1 year ago

Even setting aside whether this is desired, two issues immediately come to mind. The first is that any code like Duration::seconds(0) would break due to inference. The other is that .into() is a trait method that cannot currently be performed in const contexts. As such, this change could not be made even if it were desired (which is questionable in my opinion).