jonathanstrong commented

(Migrated from github.com)

I'm submitting a feature.

Description

Adds feature-gated support for converting v1-embedded timestamps to a chrono::DateTime<Utc> via Uuid::to_utc. Relies on underlying functionality in Uuid::to_timestamp and mimics its signature. Attempted to match existing design for feature-gated components (e.g. slog_support module) as much as possible. Tests cover post-1970 dates, pre-1970 dates, and check that v3/v4/v5 variants return None.

Rationale

chrono is ubiquitous in rust ecosystem. It's convenient to return the time in a widely-used and well-designed datetime type.

I'm submitting a feature. # Description Adds feature-gated support for converting v1-embedded timestamps to a `chrono::DateTime<Utc>` via `Uuid::to_utc`. Relies on underlying functionality in `Uuid::to_timestamp` and mimics its signature. Attempted to match existing design for feature-gated components (e.g. `slog_support` module) as much as possible. Tests cover post-1970 dates, pre-1970 dates, and check that v3/v4/v5 variants return None. # Rationale chrono is ubiquitous in rust ecosystem. It's convenient to return the time in a widely-used and well-designed datetime type.

KodrAus commented

2019-05-10 01:24:18 -05:00

(Migrated from github.com)

Thanks for the PR @jonathanstrong!

This looks like a well tested piece of code, however I don’t think we should necessarily provide and support this in uuid. It could be offered in an external crate instead.

Thanks for the PR @jonathanstrong! This looks like a well tested piece of code, however I don’t think we should necessarily provide and support this in `uuid`. It could be offered in an external crate instead.

👍 1

jstrong-tios commented

2019-05-10 10:02:51 -05:00

(Migrated from github.com)

You're welcome - happy to help!

If I may, I'd like to present a more detailed case for its inclusion, which perhaps should have accompanied the original pull request:

Imagine you're coming at this fresh: you have some V1 UUIDs with embedded timestamps, and you need to get them out. Pretty soon you realize the time comes in an unusual, esoteric format (as timestamps go) -- the number of 100ns intervals since 1582-10-15. Where would you go looking for an existing implementation to convert it to something that's actually usable?

If you doubt that the V1 timestamp format is the ugly duckling here, consider that Uuid::new_v1 assumes, without any explicit documentation, that the seconds and nano_seconds arguments it takes are "unix" timestamps - representing a duration since 1970-01-01. It's not even possible to represent a date prior to 1970 with the current implementation, since it takes seconds as a u64.

The "unix" timestamp passed to new_v1 is converted to the V1's format's scale and embedded in the resulting UUID. However, when the time comes back out (via to_timestamp), it's still in V1 format, which is practically useless in any other context.

In my mind, the only other candidate for housing this functionality, other than my application, is in chrono. But that makes considerably less sense because chrono didn't wander into uuid's neighborhood, uuid wandered into its neighborhood with to_timestamp -- except no one can understand it because it's speaking in a foreign language (the parts we can decipher are something about Pope Gregory XIII).

And on a related topic, this is chrono's neighborhood, and not the standard lib's time module, since there's no good type for representing this application of a time in the standard library. Duration is ambiguous, since it isn't explicitly tethered to any reference point. SystemTime is the only candidate, but it's unacceptably bare-bones -- no conversion functionality of any kind, or even the ability to construct it from an arbitrary calendar date. Instant is purposefully designed to be impossible to use for this.

Finally, you mentioned the pull request is well-tested. I appreciate the praise. But I'll admit, I'm far from a TDD-evangelizing, code coverage-measuring Ned Flanders of testing. It's because I've got scars from all the times I got dates and times wrong. Which brings me to my last point: it's not rocket science to convert the number of 100ns intervals since 1582 to the number of nanoseconds since 1970, but it's relatively easy to get wrong, and it takes a few minutes of careful concentration. That's exactly the type of code you want in a library. I'll be fine, I've written my implementation and I can use it from now on. But for the next guy coming along, asking himself "but why 1582-10-15?", consider saving him the trouble.

Thanks for your consideration and your hard work on this great library.

(edit: just realized I posted this with a work account. I am the original requestor.)

You're welcome - happy to help! If I may, I'd like to present a more detailed case for its inclusion, which perhaps should have accompanied the original pull request: Imagine you're coming at this fresh: you have some V1 UUIDs with embedded timestamps, and you need to get them out. Pretty soon you realize the time comes in an unusual, esoteric format (as timestamps go) -- the number of 100ns intervals since 1582-10-15. Where would you go looking for an existing implementation to convert it to something that's actually usable? If you doubt that the V1 timestamp format is the ugly duckling here, consider that `Uuid::new_v1` assumes, without any explicit documentation, that the `seconds` and `nano_seconds` arguments it takes are "unix" timestamps - representing a duration since 1970-01-01. It's not even possible to represent a date prior to 1970 with the current implementation, since it takes seconds as a `u64`. The "unix" timestamp passed to `new_v1` is converted to the V1's format's scale and embedded in the resulting UUID. However, when the time comes back out (via `to_timestamp`), it's still in V1 format, which is practically useless in any other context. In my mind, the only other candidate for housing this functionality, other than my application, is in `chrono`. But that makes considerably less sense because `chrono` didn't wander into `uuid`'s neighborhood, `uuid` wandered into *its* neighborhood with `to_timestamp` -- except no one can understand it because it's speaking in a foreign language (the parts we can decipher are something about Pope Gregory XIII). And on a related topic, this is `chrono`'s neighborhood, and not the standard lib's `time` module, since there's no good type for representing this application of a time in the standard library. `Duration` is ambiguous, since it isn't explicitly tethered to any reference point. `SystemTime` is the only candidate, but it's unacceptably bare-bones -- no conversion functionality of any kind, or even the ability to construct it from an arbitrary calendar date. `Instant` is purposefully designed to be impossible to use for this. Finally, you mentioned the pull request is well-tested. I appreciate the praise. But I'll admit, I'm far from a TDD-evangelizing, code coverage-measuring Ned Flanders of testing. It's because I've got scars from all the times I got dates and times wrong. Which brings me to my last point: it's not rocket science to convert the number of 100ns intervals since 1582 to the number of nanoseconds since 1970, but it's relatively easy to get wrong, and it takes a few minutes of careful concentration. That's exactly the type of code you want in a library. I'll be fine, I've written my implementation and I can use it from now on. But for the next guy coming along, asking himself "but why 1582-10-15?", consider saving him the trouble. Thanks for your consideration and your hard work on this great library. (edit: just realized I posted this with a work account. I am the original requestor.)

KodrAus commented

2019-05-10 18:59:52 -05:00

(Migrated from github.com)

Thanks for the extra details @jonathanstrong It sounds like we could do better with our v1 support (I don't personally use v1 UUIDs).

The main concern I have is introducing a dependency on chrono, which even though it's widely used it's not totally ubiquitous, and as I understand there are some overhauls of its API planned. I think we should be careful about pulling it in before 1.0 and painting ourselves into a corner.

What if we took this opportunity to change Uuid::to_timestamp so it's more convenient:

pub struct Timestamp {
    timestamp: u64,
    counter: u16
}

impl Uuid {
    pub fn new_v1<T>(
        context: &T,
        ts: Timestamp,
        node_id: &[u8],
    ) -> Result<Self, crate::BytesError>
    where
        T: ClockSequence,
    {
        ..
    }

    pub fn to_timestamp(&self) -> Option<Timestamp> { .. }
}

impl Timestamp {
    pub fn from_rfc4122(ts: u64, counter: u16) -> Self { .. }

    pub fn from_unix(secs: u64, nanos: u32) -> Self { .. }

    pub fn to_rfc4122(&self) -> (u64, u16) { .. }

    pub fn to_unix(&self) -> (u64, u32) { .. }
}

That way we could be consistent in the shape of the timestamp passed in and out, and manage the trickier conversion from RFC4122 timestamps into UNIX timestamps for wrapping up in chrono.

What do you think?

Thanks for the extra details @jonathanstrong It sounds like we could do better with our v1 support (I don't personally use v1 UUIDs). The main concern I have is introducing a dependency on `chrono`, which even though it's widely used it's not totally ubiquitous, and as I understand there are some overhauls of its API planned. I think we should be careful about pulling it in before `1.0` and painting ourselves into a corner. What if we took this opportunity to change `Uuid::to_timestamp` so it's more convenient: ```rust pub struct Timestamp { timestamp: u64, counter: u16 } impl Uuid { pub fn new_v1<T>( context: &T, ts: Timestamp, node_id: &[u8], ) -> Result<Self, crate::BytesError> where T: ClockSequence, { .. } pub fn to_timestamp(&self) -> Option<Timestamp> { .. } } impl Timestamp { pub fn from_rfc4122(ts: u64, counter: u16) -> Self { .. } pub fn from_unix(secs: u64, nanos: u32) -> Self { .. } pub fn to_rfc4122(&self) -> (u64, u16) { .. } pub fn to_unix(&self) -> (u64, u32) { .. } } ``` That way we could be consistent in the shape of the timestamp passed in and out, and manage the trickier conversion from RFC4122 timestamps into UNIX timestamps for wrapping up in `chrono`. What do you think?

❤️ 1 🎉 1

KodrAus commented

2019-05-10 19:21:36 -05:00

(Migrated from github.com)

It might need a little more thought with the Context type though, which generates a sequence number based on an input UNIX timestamp 🤔

It might need a little more thought with the `Context` type though, which generates a sequence number based on an input UNIX timestamp 🤔

KodrAus commented

2019-05-10 19:46:10 -05:00

(Migrated from github.com)

Could be:

pub struct Timestamp {
    timestamp: u64,
    counter: u16
}

impl Uuid {
    pub fn new_v1(
        ts: Timestamp,
        node_id: &[u8],
    ) -> Result<Self, crate::BytesError> {
        ..
    }

    pub fn to_timestamp(&self) -> Option<Timestamp> { .. }
}

impl Timestamp {
    pub fn from_rfc4122(ts: u64, counter: u16) -> Self { .. }

    pub fn from_unix<T: ClockSequence>(secs: u64, nanos: u32, context: &T) -> Self { .. }

    pub fn to_rfc4122(&self) -> (u64, u16) { .. }

    pub fn to_unix(&self) -> (u64, u32) { .. }
}

Could be: ```rust pub struct Timestamp { timestamp: u64, counter: u16 } impl Uuid { pub fn new_v1( ts: Timestamp, node_id: &[u8], ) -> Result<Self, crate::BytesError> { .. } pub fn to_timestamp(&self) -> Option<Timestamp> { .. } } impl Timestamp { pub fn from_rfc4122(ts: u64, counter: u16) -> Self { .. } pub fn from_unix<T: ClockSequence>(secs: u64, nanos: u32, context: &T) -> Self { .. } pub fn to_rfc4122(&self) -> (u64, u16) { .. } pub fn to_unix(&self) -> (u64, u32) { .. } } ```

kinggoesgaming commented

2019-05-12 01:57:53 -05:00

(Migrated from github.com)

chrono is ubiquitous in rust ecosystem.

chrono maybe the defacto across the rust community, but uuid cannot have the implementation this PR currently provides, as it violates the C-STABLE API guideline (see #191).

I would suggest on following on what @KodrAus has proposed above

> chrono is ubiquitous in rust ecosystem. `chrono` maybe the defacto across the rust community, but `uuid` cannot have the implementation this PR currently provides, as it violates the `C-STABLE` API guideline (see #191). I would suggest on following on what @KodrAus has proposed above

jonathanstrong commented

2019-05-13 06:21:37 -05:00

(Migrated from github.com)

The alternative api proposed by @KodrAus is a huge improvement over the status quo, from my perspective. I'd be happy to file an alternative pull request matching it, but it may take a few days for me to get to it.

On a related note, do you have any further information about the changes coming to chrono? I'm planning to file an issue to inquire about path to v1.0 and what still needs to be done.

The alternative api proposed by @KodrAus is a huge improvement over the status quo, from my perspective. I'd be happy to file an alternative pull request matching it, but it may take a few days for me to get to it. On a related note, do you have any further information about the changes coming to `chrono`? I'm planning to file an issue to inquire about path to v1.0 and what still needs to be done.

👍 2

jonathanstrong commented

2019-05-30 23:35:23 -05:00

(Migrated from github.com)

fyi - pushed draft implementation on a different branch here. Still needs tests, but thought I would solicit early feedback and check-in. It follows the proposed api discussed here pretty closely.

fyi - pushed draft implementation on a different branch [here](https://github.com/jonathanstrong/uuid/commit/6ab9a47eeaf94d136601563cbf00b73a33e45b5f). Still needs tests, but thought I would solicit early feedback and check-in. It follows the proposed api discussed here pretty closely.

kinggoesgaming commented

2019-05-31 01:37:57 -05:00

(Migrated from github.com)

cc @Dylan-DPC @KodrAus

KodrAus commented

2019-10-01 23:45:20 -05:00

(Migrated from github.com)

Hi @jonathanstrong 👋

Thanks for working on this! I'm working through some breaking changes and cleanups in #427.

Your proposed draft implementation looks good to me! So I've cherry-picked it into #427 and made a few tweaks, which you can see here.

Hi @jonathanstrong :wave: Thanks for working on this! I'm working through some breaking changes and cleanups in #427. Your proposed draft implementation looks good to me! So I've cherry-picked it into #427 and made a few tweaks, which you can see [here](https://github.com/uuid-rs/uuid/blob/35d4ef46841237d44aff99bd821a974293b118cf/src/v1.rs).

KodrAus commented

2019-10-04 18:39:30 -05:00

(Migrated from github.com)

Alrighty, now that #427 is merged I think we can close this one, since it includes an implementation of this feature.

Thanks again for working on this @jonathanstrong!

Alrighty, now that #427 is merged I think we can close this one, since it includes an implementation of this feature. Thanks again for working on this @jonathanstrong!

jonathanstrong commented

2019-10-04 18:42:07 -05:00

(Migrated from github.com)

@KodrAus definitely - appreciate you working with me to refine the idea and merging the code! Huge improvement for working with v1 embedded timestamps.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

add optional chrono support (Uuid::to_utc() -> Option<DateTime<Utc>>) #405

Description

Rationale

Pull request closed

add optional chrono support (`Uuid::to_utc() -> Option<DateTime<Utc>>`) #405