from_rng_bytes added #172

2018-03-17T11:02:28-05:00

Dylan-DPC-zz commented

2018-03-17 11:02:28 -05:00

(Migrated from github.com)

I'm submitting a ...

bug fix
feature enhancement
deprecation or removal
refactor

Description

As discussed in the issue, this method is a solution to avoid making rand a public dependency

Motivation

To avoid making rand public dependency

Tests

none, test added

#170

**I'm submitting a ...** - [ ] bug fix - [x] feature enhancement - [ ] deprecation or removal - [ ] refactor # Description  As discussed in the issue, this method is a solution to avoid making `rand` a public dependency # Motivation To avoid making `rand` public dependency  # Tests  none, test added # Related Issue(s) #170

👍 1

radix (Migrated from github.com) reviewed

2018-03-17 11:02:28 -05:00

kinggoesgaming (Migrated from github.com) requested changes

2018-03-17 13:09:20 -05:00

src/lib.rs

					
				@ -1807,0 +1828,4 @@

				    fn test_from_random_bytes() {

				        let b = [

				            0xa1, 0xa2, 0xa3, 0xa4, 0xb1, 0xb2, 0xc1, 0xc2, 0xd1, 0xd2, 0xd3, 0xd4, 0xd5, 0xd6,

				            0xd7, 0xd8,

Add space before {, rustfmt is complaining

Add space before `{`, rustfmt is complaining

kinggoesgaming (Migrated from github.com) approved these changes

2018-03-17 14:04:33 -05:00

KodrAus (Migrated from github.com) reviewed

2018-03-17 19:49:50 -05:00

src/lib.rs

					
				@ -685,6 +684,33 @@ impl Uuid {

				        Uuid { bytes: b }

				    }

We also need to call set_variant and set_version here since the random bytes probably won't have the right values:

let mut uuid = Uuid { bytes: [0; 16] };
copy_memory(&mut uuid.bytes, &b);

uuid.set_variant(UuidVariant::RFC4122);
uuid.set_version(UuidVersion::Random);

uuid

We also need to call `set_variant` and `set_version` here since the random bytes probably won't have the right values: ```rust let mut uuid = Uuid { bytes: [0; 16] }; copy_memory(&mut uuid.bytes, &b); uuid.set_variant(UuidVariant::RFC4122); uuid.set_version(UuidVersion::Random); uuid ```

KodrAus commented

2018-03-17 20:06:19 -05:00

(Migrated from github.com)

🚲🏚: I think I also agree with @nox's suggestion of using from_random_bytes instead of from_rng_bytes, since they're not really random number generator bytes, they're just random bytes.

🚲🏚: I think I also agree with @nox's suggestion of using `from_random_bytes` instead of `from_rng_bytes`, since they're not really _random number generator bytes_, they're just _random bytes_.

nox commented

2018-03-18 03:15:16 -05:00

(Migrated from github.com)

Neither from_rng_bytes nor from_random_bytes convey that we are building a v4 UUID, and those names are inconsistent with Uuid::new_v4.

Neither `from_rng_bytes` nor `from_random_bytes` convey that we are building a `v4` UUID, and those names are inconsistent with `Uuid::new_v4`.

KodrAus commented

2018-03-18 03:19:23 -05:00

(Migrated from github.com)

Hmm, I think Uuid::from_random_bytes(b) conveys that at least as well as rng.gen::<Uuid>() did.

Hmm, I think `Uuid::from_random_bytes(b)` conveys that at least as well as `rng.gen::<Uuid>()` did.

nox commented

2018-03-18 06:21:39 -05:00

(Migrated from github.com)

This PR should include a patch to Uuid::new_v4 to make it call the new method we are introducing.

This PR should include a patch to `Uuid::new_v4` to make it call the new method we are introducing.

nox (Migrated from github.com) reviewed

2018-03-18 06:47:44 -05:00

src/lib.rs

					
				@ -685,6 +684,33 @@ impl Uuid {

				        Uuid { bytes: b }

				    }

There is no reason to feature-gate the method at all.

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

2018-03-18 06:49:37 -05:00

src/lib.rs

					
				@ -685,6 +684,33 @@ impl Uuid {

				        Uuid { bytes: b }

				    }

We are only generating v4 uuids from this. So why have it on other features?

nox (Migrated from github.com) reviewed

2018-03-18 06:51:39 -05:00

src/lib.rs

					
				@ -685,6 +684,33 @@ impl Uuid {

				        Uuid { bytes: b }

				    }

Because there is no reason to have to bring in rand if one does not need it. For example in Servo we won't use new_v4 at all (and thus the rand in uuid) because we need a better RNG.

Because there is no reason to have to bring in `rand` if one does not need it. For example in Servo we won't use `new_v4` at all (and thus the `rand` in `uuid`) because we need a better RNG.

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

2018-03-18 07:01:31 -05:00

src/lib.rs

					
				@ -685,6 +684,33 @@ impl Uuid {

				        Uuid { bytes: b }

				    }

ok Cool.

KodrAus (Migrated from github.com) reviewed

2018-03-19 17:13:23 -05:00

KodrAus (Migrated from github.com) left a comment

Thanks @Dylan-DPC! Just a few nitpicks.

src/lib.rs

					
				@ -685,6 +684,33 @@ impl Uuid {

				        Uuid { bytes: b }

This wil need to be rebased now since copy_memory is now gone.

This wil need to be rebased now since `copy_memory` is now gone.

Probably don't need to reference GitHub issues in the docs prose.

src/lib.rs

I still think we should add an example that's a little more motivating for people using this function, I don't think the value of the bytes we pass in, nor the assertion, should be visible in the remdered docs.

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

2018-03-19 17:15:10 -05:00

src/lib.rs

I just copied it from the examples given for the other functions. Will change it accordingly.

Edit: It is common to display the assertion in rust examples. I have seen it in other crates as well and we are doing it for the other examples too.

I just copied it from the examples given for the other functions. Will change it accordingly. Edit: It is common to display the assertion in rust examples. I have seen it in other crates as well and we are doing it for the other examples too.

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

2018-03-19 17:20:34 -05:00

src/lib.rs

					
				@ -685,6 +684,33 @@ impl Uuid {

				        Uuid { bytes: b }

sure. Since we have so many confusing functions to generate a UUID wanted to leave a note on why do we need this one 😄 will remove it

sure. Since we have so many confusing functions to generate a UUID wanted to leave a note on why do we need this one :smile: will remove it

kinggoesgaming commented

2018-03-19 23:12:03 -05:00

(Migrated from github.com)

@KodrAus any further nits? Else can you approve?

KodrAus (Migrated from github.com) reviewed

2018-03-19 23:45:20 -05:00

src/lib.rs

					
				@ -685,6 +684,33 @@ impl Uuid {

				        Uuid { bytes: b }

We don't need to create a new empty slice here and copy b into it, we could just create a Uuid { bytes: b }.

We don't need to create a new empty slice here and copy `b` into it, we could just create a `Uuid { bytes: b }`.

KodrAus (Migrated from github.com) approved these changes

2018-03-20 16:28:48 -05:00

KodrAus (Migrated from github.com) left a comment

Thanks @Dylan-DPC! Looks good to me!

Dylan-DPC-zz commented

2018-03-20 16:30:08 -05:00

(Migrated from github.com)

bors: r+

❤️ 1

kinggoesgaming commented

2018-03-20 16:45:21 -05:00

(Migrated from github.com)

bors: r+

bors[bot] commented

2018-03-20 16:53:11 -05:00

(Migrated from github.com)

Build succeeded

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

Sign in to join this conversation.

No reviewers

radix

kinggoesgaming

KodrAus