Add from_uuid_bytes #145

2018-02-11T16:52:56-06:00

ncallaway commented

2018-02-11 16:52:56 -06:00

(Migrated from github.com)

This adds a constructor that uses the explicit type UuidBytes.

The main difference between this constructor and from_bytes is that the length of UuidBytes is fixed at the correct length, so any use of from_uuid_bytes cannot fail. As such, instead of returning a Result it simply returns a uuid.

A secondary reason for this is that, if const functions ever move into nightly, this function would be an easy way to allow external consumers of this library to create const UUIDs.

If there are any other tests, or contribution guidelines that I've failed to include, please let me know.

This adds a constructor that uses the explicit type `UuidBytes`. The main difference between this constructor and `from_bytes` is that the length of `UuidBytes` is fixed at the correct length, so any use of `from_uuid_bytes` cannot fail. As such, instead of returning a `Result` it simply returns a `uuid`. A secondary reason for this is that, if `const` functions ever move into nightly, this function would be an easy way to allow external consumers of this library to create `const` UUIDs. If there are any other tests, or contribution guidelines that I've failed to include, please let me know.

radix (Migrated from github.com) reviewed

2018-02-11 16:52:56 -06:00

kinggoesgaming (Migrated from github.com) reviewed

2018-02-11 19:59:10 -06:00

kinggoesgaming commented

2018-02-11 22:17:48 -06:00

(Migrated from github.com)

@ncallaway other than the review can you run cargo +nightly fmt aswell?

@ncallaway other than the review can you run `cargo +nightly fmt` aswell?

Dylan-DPC-zz (Migrated from github.com) approved these changes

2018-02-14 05:36:37 -06:00

KodrAus (Migrated from github.com) reviewed

2018-02-15 04:17:33 -06:00

src/lib.rs

					
				@ -603,6 +603,40 @@ impl Uuid {

				        Ok(uuid)

Since we just take a copy of these bytes, and UuidBytes is sized, maybe this could just take an owned UuidBytes instead of a &UuidBytes here? The clone will be really cheap, but it could be clearer that we take ownership of the raw bytes.

What do you think?

Since we just take a copy of these bytes, and `UuidBytes` is sized, maybe this could just take an owned `UuidBytes` instead of a `&UuidBytes` here? The clone will be really cheap, but it could be clearer that we take ownership of the raw bytes. What do you think?

Dylan-DPC-zz (Migrated from github.com) reviewed

2018-02-15 06:28:59 -06:00

src/lib.rs

					
				@ -603,6 +603,40 @@ impl Uuid {

				        Ok(uuid)

from_bytes() also accepts a reference. So this keeps it consistent with that. Also I guess it is better to change the return type to a Result right?

`from_bytes()` also accepts a reference. So this keeps it consistent with that. Also I guess it is better to change the return type to a `Result` right?

KodrAus (Migrated from github.com) reviewed

2018-02-15 16:07:43 -06:00

src/lib.rs

					
				@ -603,6 +603,40 @@ impl Uuid {

				        Ok(uuid)

Hmm, from_bytes takes a reference because it needs to, otherwise it's unsized. But UuidBytes isn't unsized, so we can take it by value if we want, since we need it by value.

I don't think we'd need to return a Result, because this always succeeds.

Hmm, `from_bytes` takes a reference because it needs to, otherwise it's unsized. But `UuidBytes` isn't unsized, so we can take it by value if we want, since we need it by value. I don't think we'd need to return a `Result`, because this always succeeds.

ncallaway (Migrated from github.com) reviewed

2018-02-15 20:31:21 -06:00

src/lib.rs

					
				@ -603,6 +603,40 @@ impl Uuid {

				        Ok(uuid)

Yes, I deliberately didn't return a Result, because this always succeeds, as KodrAus noted. One motivation for this function (for me) is that I want to be able to create const Uuid's (if/when const functions come out of Rust nightly). I believe (thought I'm not positive) that if this returns a Result, this function wouldn't be able to become a const function that could generate a const Uuid.

I did have a reference in the signature to mirror from_bytes(), as noted by Dylan-DPC. However, I didn't have a strong motivational reason for this, other than matching the interface. I think KodrAus makes a good point that it might be better than the consumer of this function should be aware that a copy of the bytes are going to happen. As such, I'd be happy to change the interface to take an owned UuidBytes, and require the caller to perform the .clone if they want the copy.

I can make that change tomorrow morning (PST) and send the results. I can include the nightly rustfmt change at that time, too.

Yes, I deliberately didn't return a `Result`, because this always succeeds, as KodrAus noted. One motivation for this function (for me) is that I want to be able to create const Uuid's (if/when const functions come out of Rust nightly). I believe (thought I'm not positive) that if this returns a Result, this function wouldn't be able to become a const function that could generate a const Uuid. I did have a reference in the signature to mirror `from_bytes()`, as noted by Dylan-DPC. However, I didn't have a strong motivational reason for this, other than matching the interface. I think KodrAus makes a good point that it might be better than the consumer of this function should be aware that a copy of the bytes are going to happen. As such, I'd be happy to change the interface to take an owned UuidBytes, and require the caller to perform the `.clone` if they want the copy. I can make that change tomorrow morning (PST) and send the results. I can include the nightly rustfmt change at that time, too.

Dylan-DPC-zz (Migrated from github.com) reviewed

2018-02-15 21:58:53 -06:00

src/lib.rs

					
				@ -603,6 +603,40 @@ impl Uuid {

				        Ok(uuid)

Yeah i agree with it. Cool

ncallaway commented

2018-02-16 18:41:00 -06:00

(Migrated from github.com)

As discussed, this PR now has ::from_uuid_bytes take an owned UuidBytes, instead of a reference. cargo +nightly fmt has been run as well.

As discussed, this PR now has `::from_uuid_bytes` take an owned UuidBytes, instead of a reference. `cargo +nightly fmt` has been run as well.

kinggoesgaming (Migrated from github.com) approved these changes

2018-02-16 20:28:37 -06:00

Dylan-DPC-zz commented

2018-02-17 01:54:39 -06:00

(Migrated from github.com)

bors: r+

bors[bot] commented

2018-02-17 02:01:39 -06:00

(Migrated from github.com)

Build succeeded

# Build succeeded * [continuous-integration/appveyor/branch](https://ci.appveyor.com/project/KodrAus/uuid/build/1.0.144) * [continuous-integration/travis-ci/push](https://travis-ci.org/uuid-rs/uuid/builds/342675801?utm_source=github_status&utm_medium=notification)

Sign in to join this conversation.

No reviewers

radix

Dylan-DPC-zz

kinggoesgaming