Add Uuid Builder #332

Veykril · 2018-10-02T09:07:23Z

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

KodrAus · 2018-10-02T09:55:05Z

src/builder.rs

+use super::Version;
+
+/// A builder struct for creating a [`Uuid`]
+#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]


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

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.

Actually removing the copy implementation will trigger the missing_copy_implementations lint.

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.

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.

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

KodrAus · 2018-10-02T09:57:48Z

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.

Veykril · 2018-10-02T10:34:05Z

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

Dylan-DPC-zz · 2018-10-02T10:36:07Z

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 ***@***.***> 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 <#332 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALzEtR-S-qrWTiXaSvCwO82aJWvZ8ZxOks5ug0EigaJpZM4XDnvz> .

KodrAus · 2018-10-02T12:28:37Z

@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 · 2018-10-02T12:30:48Z

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

Veykril · 2018-10-02T16:49:45Z

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

Dylan-DPC-zz · 2018-10-02T16:50:18Z

yep @Veykril that's fine

kinggoesgaming · 2018-10-02T19:29:34Z

src/builder.rs

@@ -0,0 +1,238 @@
+use super::Bytes;
+use super::BytesError;
+use super::Uuid;


use uuid::prelude::* where possible

kinggoesgaming · 2018-10-02T19:33:46Z

src/lib.rs

@@ -137,6 +137,9 @@ extern crate sha1;
 #[cfg_attr(test, macro_use)]
 extern crate slog;

+mod builder;
+pub use builder::UuidBuilder;


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

kinggoesgaming · 2018-10-02T19:36:08Z

src/lib.rs

@@ -619,6 +622,10 @@ impl Uuid {
    ///
    /// assert_eq!(expected_uuid, uuid);
    /// ```
+    #[deprecated(
+        since = "0.7.2",
+        note = "please use the `UuidBuilder` instead"


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

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

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

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

kinggoesgaming · 2018-10-02T19:40:23Z

src/builder.rs

+/// A builder struct for creating a [`Uuid`]
+#[allow(missing_copy_implementations)]
+#[derive(Debug)]
+pub struct UuidBuilder {


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

kinggoesgaming · 2018-10-02T19:47:45Z

src/builder.rs

+    ///     "00000000-0000-0000-0000-000000000000"
+    /// );
+    /// ```
+    pub fn build(&self) -> Uuid {


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

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.

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 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.

kinggoesgaming · 2018-10-02T19:50:33Z

src/builder.rs

+    ///
+    /// assert!(builder.is_err());
+    /// ```
+    pub fn from_slice(b: &[u8]) -> Result<Self, BytesError> {


maybe have two be/le from_slice functions instead?

kinggoesgaming · 2018-10-03T01:24:33Z

src/builder.rs

@@ -0,0 +1,238 @@
+use super::Bytes;


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

Veykril · 2018-10-05T14:59:04Z

I think I changed everything that was requested now

kinggoesgaming · 2018-10-05T15:37:17Z

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

kinggoesgaming · 2018-10-05T16:13:21Z

I dont know what that error message is 😕

@Dylan-DPC @KodrAus ideas?

Veykril · 2018-10-05T16:29:44Z

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

Dylan-DPC-zz · 2018-10-05T16:41:30Z

bors: r+

332: Add Uuid Builder r=Dylan-DPC a=Veykril **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 Co-authored-by: Lukas Wirth <[email protected]> Co-authored-by: Dylan DPC <[email protected]>

bors · 2018-10-05T16:50:14Z

Build succeeded

Add Uuid Builder

e2efb68

Veykril requested review from Dylan-DPC-zz, kinggoesgaming, KodrAus and radix as code owners October 2, 2018 09:07

KodrAus reviewed Oct 2, 2018

View reviewed changes

Veykril added 2 commits October 2, 2018 18:48

deprecate from_random_bytes

71ab7f6

rustfmt

8819ad7

fix doc tests

6cba4cb

Dylan-DPC-zz added Type:Enhancement labels Oct 2, 2018

Dylan-DPC-zz added this to the 0.7.2 milestone Oct 2, 2018

kinggoesgaming requested changes Oct 2, 2018

View reviewed changes

kinggoesgaming requested changes Oct 3, 2018

View reviewed changes

src/builder.rs Outdated

@@ -0,0 +1,238 @@

use super::Bytes;

Copy link

Member

kinggoesgaming Oct 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

kinggoesgaming added the hacktoberfest label Oct 3, 2018

Dylan-DPC and others added 5 commits October 4, 2018 04:05

Merge branch 'master' into master

7e884c4

requested changes

ec57823

Merge branch 'master' of https://github.com/Veykril/uuid

de61620

Merge branch 'master' into master

2c71d33

take self by mut on build

9d28fb3

Dylan-DPC-zz approved these changes Oct 5, 2018

View reviewed changes

kinggoesgaming approved these changes Oct 5, 2018

View reviewed changes

fix doc tests

8ef605f

Dylan-DPC-zz approved these changes Oct 5, 2018

View reviewed changes

remove nested import group

4f0caa5

bors bot merged commit 4f0caa5 into uuid-rs:master Oct 5, 2018

kinggoesgaming mentioned this pull request Oct 5, 2018

Builder pattern #171

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Uuid Builder #332

Add Uuid Builder #332

Veykril commented Oct 2, 2018

KodrAus Oct 2, 2018

Veykril Oct 2, 2018

Veykril Oct 2, 2018

KodrAus Oct 2, 2018 •

edited

Loading

KodrAus Oct 2, 2018

Veykril Oct 2, 2018

KodrAus commented Oct 2, 2018

Veykril commented Oct 2, 2018

Dylan-DPC-zz commented Oct 2, 2018 via email

KodrAus commented Oct 2, 2018

Dylan-DPC-zz commented Oct 2, 2018

Veykril commented Oct 2, 2018

Dylan-DPC-zz commented Oct 2, 2018

kinggoesgaming Oct 2, 2018

kinggoesgaming Oct 2, 2018

kinggoesgaming Oct 2, 2018

KodrAus Oct 3, 2018

Dylan-DPC-zz Oct 3, 2018

Veykril Oct 3, 2018

kinggoesgaming Oct 2, 2018

kinggoesgaming Oct 2, 2018

KodrAus Oct 3, 2018

Veykril Oct 3, 2018

KodrAus Oct 3, 2018

kinggoesgaming Oct 2, 2018

kinggoesgaming Oct 3, 2018

Veykril commented Oct 5, 2018

kinggoesgaming commented Oct 5, 2018

kinggoesgaming commented Oct 5, 2018

Veykril commented Oct 5, 2018

Dylan-DPC-zz commented Oct 5, 2018

bors bot commented Oct 5, 2018

Add Uuid Builder #332

Add Uuid Builder #332

Conversation

Veykril commented Oct 2, 2018

Description

Motivation

Tests

Related Issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KodrAus Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KodrAus commented Oct 2, 2018

Veykril commented Oct 2, 2018

Dylan-DPC-zz commented Oct 2, 2018 via email

KodrAus commented Oct 2, 2018

Dylan-DPC-zz commented Oct 2, 2018

Veykril commented Oct 2, 2018

Dylan-DPC-zz commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Veykril commented Oct 5, 2018

kinggoesgaming commented Oct 5, 2018

kinggoesgaming commented Oct 5, 2018

Veykril commented Oct 5, 2018

Dylan-DPC-zz commented Oct 5, 2018

bors bot commented Oct 5, 2018

Build succeeded

KodrAus Oct 2, 2018 •

edited

Loading