feat: impl TryFrom str for Timestamp

[tisonkun]

Not quite sure if this falls into our API philosophy.

My use case is such a method (full code):

    pub fn find_next<T>(&self, timestamp: T) -> Result<Zoned, Error>
    where
        T: TryInto<Timestamp>,
        T::Error: std::error::Error,
    {
        let zoned = timestamp
            .try_into()
            .map(|ts| ts.to_zoned(self.timezone.clone()))
            .map_err(error_with_context("failed to parse timestamp"))?;
        // ...
    }

... and I want foo.find_next("2024-01-01T00:00:00+08:00") compile.

The reason for such a method is:

1. Accept both Timestamp and things that can convert to Timestamp.
2. Why not directly Timestamp? Because I don't want users to that crate have to explicit add a dependency to jiff (not only the extra typing and concept, but also version resolution puzzle like https://github.com/GreptimeTeam/greptimedb/issues/3610).

Alternatives. I can switch to:

1. Directly depend on jiff anyway.
2. Expose different methods to accept common inputs (Timestamp, String, and even millis)
3. Accept a closure that construct Timestamp fallibly. So || "...".parse() and || Ok(ts) are accepted.

[tisonkun]

Or I can add a wrapper type MakeTimestamp in downstream. However, I'm still wondering the philosophy difference between implement FromStr and TryFrom<&str>.

The former can enable the "...".parse() pattern. But why not always TryFrom<&str> for those implementing FromStr.

[tisonkun]

Adopt https://github.com/BurntSushi/jiff/pull/137#issuecomment-2369922755 so not an necessary patch for me.

Close since it doesn't seems follow the conversion flavor. But I'd still like to know the reason and if this is valid, we can reopen anyway.

[BurntSushi]

The missing TryFrom impl I think roughly follows what we do in std. For example, IpAddr impls FromStr but not TryFrom. As for why that is, I'm not entirely sure it's a settled question to be honest.

But popping up a level, your stated motivation is not quite right:

Why not directly Timestamp? Because I don't want users to that crate have to explicit add a dependency to jiff

The API you've provided, regardless of whether you accept TryInto<Timestamp> or a Timestamp directly, already implies a public dependency on jiff. So using TryFrom quite literally does nothing to escape the issues like the one you linked. Best practice for public dependencies is to re-export them at the crate root so that users of your library don't need to directly depend on them.

[BurntSushi]

As for the motivation of making the call site more succinct, there are a variety of ways to tackle that. Jiff itself uses various techniques that you might learn from.

[tisonkun]

using TryFrom quite literally does nothing to escape the issues

As supporting other TryFrom variant like TryFrom<&'a str> and direct constructor MakeTimestamp::from_nanosecond etc. It can escape the issues when jiff cannot resolve to the same version.

And yes, re-export is possible, while I'm not sure the range of the exported struct so I try to use an alternative.

there are a variety of ways to tackle that

Yes. I saw some of them. But I can't sort them out currently .. Would you point me some typical methods or is there any existing note on solving this kind of requirement?

[BurntSushi]

Sorry, it's very hard for me to understand your English. For example, I don't know what you mean by "I'm not sure the range of the exported struct."

Would you point me some typical methods or is there any existing note on solving this kind of requirement?

Like I said in the other issue, I don't really have a ton of time to do abstract mentoring. It sounds like you already learned on approach. There are other approaches with varying trade offs. Like custom traits. API design is a very big topic, and there just aren't readily accessible materials that I'm aware of that explain everything.

[tisonkun]

I don't really have a ton of time to do abstract mentoring

Got it and thanks for all your time.

[tisonkun]

I'm not sure the range of the exported struct.

In trying to improve the expression ...

The first thing I tried was re-export Timestamp. But I don't have much experience deciding what should be re-exported in a library. For example, should I re-export jiff::Timestamp only? Or jiff:Zoned also? If some methods that Timestamp implements are defined in a jiff's trait, should I later export the trait or pub use jiff::* at all?

This may sound like unfounded worry, but I'm still learning the boundaries and best practices. Since using a wrapper fits my use case, I'm using it instead.

#[derive(Debug, Copy, Clone)]
pub struct MakeTimestamp(pub Timestamp);

impl From<Timestamp> for MakeTimestamp { ... }
impl FromStr for MakeTimestamp { ... }
impl<'a> TryFrom<&'a str> for MakeTimestamp { ... }
impl MakeTimestamp {
  pub fn from_second(second: i64) -> Result<Self, Error> { ... }
  pub fn from_millisecond(millisecond: i64) -> Result<Self, Error> { ... }
  pub fn from_microsecond(microsecond: i64) -> Result<Self, Error> { ... }
  pub fn from_nanosecond(nanosecond: i128) -> Result<Self, Error> { ... }
}

The API you've provided, regardless of whether you accept TryInto or a Timestamp directly, already implies a public dependency on jiff.

I got you now. You mean the API implemented in this PR, and that's correct.

With impl<'a> TryFrom<&'a str> for jiff::Timestamp { ... }, if users simply pass a string to the parameter accepts TryInto<Timestamp>, they may not need to add jiff in the Cargo.toml dependency, and the version of Timestamp here is the one dependent by downstream lib. But if users want to generify it, when they bound their variables with TryInto<jiff::Timestamp>, it refers to the jiff in their dependency tree, so it's the same as depending on jiff::Timestamp.

This can be subtly confusing also. So you're right that the way proposed at the beginning is not quite right.

[BurntSushi]

For re-exporting, you should be doing pub extern crate jiff; in the crate root. Or similar. See how rkyv does it: https://docs.rs/rkyv/latest/rkyv/#re-exports

You mean the API implemented in this PR, and that's correct.

No... I mean your API you have as a motivating example. You're returning a Zoned. Therefore, Jiff is already a public dependency by virtue of that alone. The TryInto or whatever is irrelevant.

And again, as long as you re-export the entire Jiff crate, which is standard practice for public dependencies, callers don't need to add an explicit dependency on Jiff. See https://www.lurklurk.org/effective-rust/re-export.html and https://github.com/rust-lang/api-guidelines/discussions/176

Of course, it would be better not to make Jiff a public dependency in the first place. So you're right to chase wrapper types for Timestamp and what not. But your motivating example would not avoid the public dependency on Jiff because you still have a return type that references an item in Jiff's API.

Maybe you're confused about what a public dependency is. I'm not sure. Basically, if Jiff types appear anywhere in the API of your library, then Jiff is a public dependency.

[tisonkun]

Maybe you're confused about what a public dependency is. I'm not sure. Basically, if Jiff types appear anywhere in the API of your library, then Jiff is a public dependency.

Oops .. Yes. As I return a Zoned, if users try to, for example, compare it with a jiff::Zoned in their dependency tree, it can cause the same version mismatch issue >_<

And thanks for the references you linked :D