Add Uuid Builder #332

2018-10-02T04:07:23-05:00

Veykril commented

2018-10-02 04:07:23 -05:00

(Migrated from github.com)

I'm submitting a feature

Description

This PR implements a basic UuidBuilder that takes itself by &mut instead of self as shown in the issue #171 description.

Motivation

Issue #171

Tests

Through Doctests

#171

**I'm submitting a feature** # Description This PR implements a basic UuidBuilder that takes itself by `&mut` instead of `self` as shown in the issue #171 description. # Motivation Issue #171 # Tests Through Doctests # Related Issue(s) #171

radix (Migrated from github.com) reviewed

2018-10-02 04:07:23 -05:00

KodrAus (Migrated from github.com) reviewed

2018-10-02 04:55:46 -05:00

src/builder.rs

					
				@ -0,0 +1,295 @@

				// Copyright 2013-2014 The Rust Project Developers.

Let's remove the Copy implementation here, because the behaviour of Copy types and &mut self methods can be surprising.

Let's remove the `Copy` implementation here, because the behaviour of `Copy` types and `&mut self` methods can be surprising.

KodrAus commented

2018-10-02 04:57:48 -05:00

(Migrated from github.com)

Hi @Veykril 👋

I'm on mobile at the moment so just left a little comment from a quick scan.

I'd be tempted to mark the from_random_bytes method as deprecated now and suggest using this builder type.

Hi @Veykril :wave: I'm on mobile at the moment so just left a little comment from a quick scan. I'd be tempted to mark the `from_random_bytes` method as deprecated now and suggest using this builder type.

Veykril (Migrated from github.com) reviewed

2018-10-02 05:32:47 -05:00

src/builder.rs

					
				@ -0,0 +1,295 @@

				// Copyright 2013-2014 The Rust Project Developers.

Oh interesting, I didn't know that this might cause weird behaviour. Will remove it then, this builder doesn't consume itself anyways so there is no need to copy it anyways.

Veykril commented

2018-10-02 05:34:05 -05:00

(Migrated from github.com)

Should I deprecate it with #[deprecated(since="0.7.2", note="please use the UuidBuilder instead")] then?

Should I deprecate it with `#[deprecated(since="0.7.2", note="please use the `UuidBuilder` instead")]` then?

👍 1

Dylan-DPC-zz commented

2018-10-02 05:36:07 -05:00

(Migrated from github.com)

I don't think it's a good idea to deprecate we are just giving the users to
use a builder pattern if they want to.

On Tue, Oct 2, 2018, 16:04 Lukas Wirth notifications@github.com wrote:

Should I deprecate it with #[deprecated(since="0.7.2", note="please use
theUuidBuilderinstead")] then?

—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
https://github.com/uuid-rs/uuid/pull/332#issuecomment-426226710, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALzEtR-S-qrWTiXaSvCwO82aJWvZ8ZxOks5ug0EigaJpZM4XDnvz
.

I don't think it's a good idea to deprecate we are just giving the users to use a builder pattern if they want to. On Tue, Oct 2, 2018, 16:04 Lukas Wirth <notifications@github.com> wrote: > Should I deprecate it with #[deprecated(since="0.7.2", note="please use > theUuidBuilderinstead")] then? > > — > You are receiving this because your review was requested. > Reply to this email directly, view it on GitHub > <https://github.com/uuid-rs/uuid/pull/332#issuecomment-426226710>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ALzEtR-S-qrWTiXaSvCwO82aJWvZ8ZxOks5ug0EigaJpZM4XDnvz> > . >

Veykril (Migrated from github.com) reviewed

2018-10-02 05:38:13 -05:00

src/builder.rs

					
				@ -0,0 +1,295 @@

				// Copyright 2013-2014 The Rust Project Developers.

Actually removing the copy implementation will trigger the missing_copy_implementations lint.

Actually removing the copy implementation will trigger the `missing_copy_implementations` lint.

KodrAus (Migrated from github.com) reviewed

2018-10-02 07:21:58 -05:00

src/builder.rs

					
				@ -0,0 +1,295 @@

				// Copyright 2013-2014 The Rust Project Developers.

Actually, we should probably go ahead and remove all of those derives besides Debug, the others don't really make sense for a builder type.

We can override that lint with #[allow(missing_copy_implementations)] alongside this #[derive] attribute.

Actually, we should probably go ahead and remove all of those derives besides `Debug`, the others don't really make sense for a builder type. We can override that lint with `#[allow(missing_copy_implementations)]` alongside this `#[derive]` attribute.

KodrAus commented

2018-10-02 07:28:37 -05:00

(Migrated from github.com)

@Dylan-DPC I think from_random_bytes was really only added specifically to call some of the mutating methods we now have on this builder without exposing them on Uuid, which is Copy. I don't think we need to hang on to APIs indefinitely when new approaches emerge, otherwise we could risk winding up with a large API surface area that's difficult to navigate. Pushing users towards the Builder for any non-standard initialisation seems like it would be helpful for users trying to find their way around.

@Dylan-DPC I think `from_random_bytes` was really only added specifically to call some of the mutating methods we now have on this builder without exposing them on `Uuid`, which is `Copy`. I don't think we need to hang on to APIs indefinitely when new approaches emerge, otherwise we could risk winding up with a large API surface area that's difficult to navigate. Pushing users towards the `Builder` for any non-standard initialisation seems like it would be helpful for users trying to find their way around.

Dylan-DPC-zz commented

2018-10-02 07:30:48 -05:00

(Migrated from github.com)

@KodrAus oops yeah just checked the function again. That's fine by me 👍

@KodrAus oops yeah just checked the function again. That's fine by me :+1:

👍 1

KodrAus (Migrated from github.com) reviewed

2018-10-02 07:32:00 -05:00

src/builder.rs

					
				@ -0,0 +1,295 @@

				// Copyright 2013-2014 The Rust Project Developers.

Oh interesting, I didn't know that this might cause weird behaviour.

Yeh, the wierdness is that a type that implements Copy can be implicitly copied around, so when you're passing the value around and calling mutating methods you can end up mutating a copy by accident.

> Oh interesting, I didn't know that this might cause weird behaviour. Yeh, the wierdness is that a type that implements `Copy` can be implicitly copied around, so when you're passing the value around and calling mutating methods you can end up mutating a copy by accident.

Veykril commented

2018-10-02 11:49:45 -05:00

(Migrated from github.com)

Alright, I made the proposed changes. Is the version I put into the attribute correct for this?

Dylan-DPC-zz commented

2018-10-02 11:50:18 -05:00

(Migrated from github.com)

yep @Veykril that's fine

Veykril (Migrated from github.com) reviewed

2018-10-02 11:50:19 -05:00

src/builder.rs

					
				@ -0,0 +1,295 @@

				// Copyright 2013-2014 The Rust Project Developers.

Ah yes, that makes never though of it like that. That is good thing to keep in mind.

kinggoesgaming (Migrated from github.com) requested changes

2018-10-02 14:58:51 -05:00

src/builder.rs

					
				@ -0,0 +1,295 @@

				// Copyright 2013-2014 The Rust Project Developers.

use uuid::prelude::* where possible

use `uuid::prelude::*` where possible

maybe make this a newtype tuple? Just to make it consistent with the rest of the crate

maybe make this a `newtype` tuple? Just to make it consistent with the rest of the crate

pub fn build(&self) -> Uuid -> pub fn build(self) -> Uuid

`pub fn build(&self) -> Uuid` -> `pub fn build(self) -> Uuid`

maybe have two be/le from_slice functions instead?

maybe have two be/le `from_slice` functions instead?

src/lib.rs

make the builder module pub and instead of the reexport here, reexport in prelude module

make the `builder` module `pub` and instead of the reexport here, reexport in `prelude` module

src/lib.rs

note = "use uuid::builder::UuidBuilder instead"

`note = "use `uuid::builder::UuidBuilder` instead"`

kinggoesgaming (Migrated from github.com) requested changes

2018-10-02 20:24:39 -05:00

src/builder.rs

					
				@ -0,0 +1,295 @@

				// Copyright 2013-2014 The Rust Project Developers.

Add the copyright header (copy the one in lib.rs, first few lines)

KodrAus (Migrated from github.com) reviewed

2018-10-03 00:08:38 -05:00

src/builder.rs

					
				@ -0,0 +1,295 @@

				// Copyright 2013-2014 The Rust Project Developers.

We could make this method take &mut self, so that we have the future option to move the internal uuid out instead of copying it.

We could make this method take `&mut self`, so that we have the future option to move the internal uuid out instead of copying it.

KodrAus (Migrated from github.com) reviewed

2018-10-03 00:09:41 -05:00

src/lib.rs

Something about uuid::builder::UuidBuilder doesn't sit right with me... We could just go with uuid::Builder?

Something about `uuid::builder::UuidBuilder` doesn't sit right with me... We could just go with `uuid::Builder`?

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

2018-10-03 05:48:57 -05:00

src/lib.rs

yeah its not like we are going to have several builders so uuid::Builder should be fine

yeah its not like we are going to have several builders so `uuid::Builder` should be fine

Veykril (Migrated from github.com) reviewed

2018-10-03 07:30:26 -05:00

src/lib.rs

So should I reexport the Builder via pub use builder::Builder; in the lib.rs as well as reexporting it through the prelude?

So should I reexport the `Builder` via `pub use builder::Builder;` in the `lib.rs` as well as reexporting it through the prelude?

Veykril (Migrated from github.com) reviewed

2018-10-03 07:33:14 -05:00

src/builder.rs

					
				@ -0,0 +1,295 @@

				// Copyright 2013-2014 The Rust Project Developers.

I used &self since that was what was given in the issue example, normally you'd consume the builder when building but since our internal type is copy both works here I'd say. What should I pick?

I used `&self` since that was what was given in the issue example, normally you'd consume the builder when building but since our internal type is copy both works here I'd say. What should I pick?

KodrAus (Migrated from github.com) reviewed

2018-10-03 16:08:37 -05:00

src/builder.rs

					
				@ -0,0 +1,295 @@

				// Copyright 2013-2014 The Rust Project Developers.

I would go with &mut self here, it's consistent with the other mutating methods we've added, and lets us do some extra work in the build method if we need it. Using &mut self over self means that we can chain the method calls in a single statement like this:

let uuid = Builder::from_slice(bytes).set_version(..).build();

If the build method took self by value then we wouldn't be able to chain it with set_version.

I would go with `&mut self` here, it's consistent with the other mutating methods we've added, and lets us do some extra work in the `build` method if we need it. Using `&mut self` over `self` means that we can chain the method calls in a single statement like this: ```rust let uuid = Builder::from_slice(bytes).set_version(..).build(); ``` If the `build` method took `self` by value then we wouldn't be able to chain it with `set_version`.

Veykril commented

2018-10-05 09:59:04 -05:00

(Migrated from github.com)

I think I changed everything that was requested now

👍 2

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

2018-10-05 10:21:29 -05:00

kinggoesgaming (Migrated from github.com) approved these changes

2018-10-05 10:36:16 -05:00

kinggoesgaming commented

2018-10-05 10:37:17 -05:00

(Migrated from github.com)

@Veykril can fix the build failures and then its good to merge

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

2018-10-05 10:48:33 -05:00

kinggoesgaming commented

2018-10-05 11:13:21 -05:00

(Migrated from github.com)

I dont know what that error message is 😕

@Dylan-DPC @KodrAus ideas?

I dont know what that error message is :confused: @Dylan-DPC @KodrAus ideas?

Veykril commented

2018-10-05 11:29:44 -05:00

(Migrated from github.com)

Oh I think Rust 1.22 doesn't support nested import groups, yeah that feature was added in 1.25.

👍 2

Dylan-DPC-zz commented

2018-10-05 11:41:30 -05:00

(Migrated from github.com)

bors: r+

bors[bot] commented

2018-10-05 11:50:14 -05:00

(Migrated from github.com)

Build succeeded

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

Sign in to join this conversation.

No reviewers

radix

kinggoesgaming

Dylan-DPC-zz