documentation: add further clarification to comments and documentation #12

trevorstarick · 2018-07-14T01:17:04Z

No description provided.

acln0 · 2018-07-14T01:23:49Z

I'm personally a fan of imperative commit titles, and the commits we have pushed so far follow this style, so I would recommend "add further clarification" instead of "added further clarification".

coveralls · 2018-07-14T01:24:18Z

Pull Request Test Coverage Report for Build 33

5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 97.531%

Totals
Change from base Build 26:	0.0%
Covered Lines:	316
Relevant Lines:	324

💛 - Coveralls

theckman · 2018-07-14T02:42:29Z

@trevorstarick Thank you for updating the PR title. When you have a moment can you please amend your commit with @acln0's suggested change as well?

theckman

This looks great, thank you for the contribution! There are a few minor tweaks I think we should look at making to be more clear, or precise, in certain areas.

Let me know what you think about the suggestions.

theckman · 2018-07-14T02:46:28Z

codec.go

-// FromBytesOrNil returns UUID converted from raw byte slice input.
-// Same behavior as FromBytes, but returns a Nil UUID on error.
+// FromBytesOrNil returns a UUID generated from the raw byte slice input.
+// Same behavior as FromBytes(), but returns a Nil UUID instead of an error.


This still reads a bit strange to me. How about something like:

This has the same behavior as FromBytes(), but returns a nil UUID instead of an error.

Should I call it a nil UUID even though it's referring to the special UUID type, Nil

theckman · 2018-07-14T02:47:22Z

codec.go

-// FromStringOrNil returns UUID parsed from string input.
-// Same behavior as FromString, but returns a Nil UUID on error.
+// FromStringOrNil returns a UUID parsed from the input string.
+// Same behavior as FromString(), but returns a Nil UUID instead of an error.


Similar as my other comment:

This has the same behavior as..

Also would you be able to lowercase "Nil"?

theckman · 2018-07-14T02:52:51Z

codec.go

@@ -108,7 +108,7 @@ func (u *UUID) UnmarshalText(text []byte) (err error) {
 	}
 }

-// decodeCanonical decodes UUID string in format
+// decodeCanonical decodes UUID strings that are formatted like the following format:


To point people to the origin of the format, would you be opposed to citing the RFC and section?

strings that are formatted as defined in RFC-4122 (section 3):

theckman · 2018-07-14T02:54:29Z

codec.go

@@ -133,8 +133,8 @@ func (u *UUID) decodeCanonical(t []byte) (err error) {
 	return
 }

-// decodeHashLike decodes UUID string in format
-// "6ba7b8109dad11d180b400c04fd430c8".
+// decodeHashLike decodes UUID strings that are formatted like the following format:


strings that are using the following format:

theckman · 2018-07-14T02:54:50Z

codec.go

-// decodeBraced decodes UUID string in format
-// "{6ba7b810-9dad-11d1-80b4-00c04fd430c8}" or in format
-// "{6ba7b8109dad11d180b400c04fd430c8}".
+// decodeBraced decodes UUID strings that are formatted like the following formats:


How do you feel about something like this for this ones, and the rest in the file?

strings that are using the following formats:

theckman · 2018-07-14T02:58:50Z

uuid.go

+//
+// RFC 4122 (https://tools.ietf.org/html/rfc4122) provides the specification for versions 1, 3, 4, and 5.
+//
+// DCE 1.1 (http://pubs.opengroup.org/onlinepubs/9696989899/chap5.htm#tagcjh_08_02_01_01) provides the specification for version 2.


Would you be willing to wrap the lines in this section to look more natural in relation to the license above?

// Package uuid provides implementations of the Universally Unique Identifier // (UUID), as specified in RFC 4122 and DCE 1.1. // // RFC 4122 (https://tools.ietf.org/html/rfc4122) provides the specification for // versions 1, 3, 4, and 5. // // DCE 1.1[1] provides the specification for version 2. // // [1] http://pubs.opengroup.org/onlinepubs/9696989899/chap5.htm#tagcjh_08_02_01_01

@trevorstarick actually, how do you feel about adding references to both?

// Package uuid provides implementations of the Universally Unique Identifier // (UUID), as specified in RFC-4122[1] and DCE 1.1[2]. // // RFC 4122 (https://tools.ietf.org/html/rfc4122) provides the specification for // versions 1, 3, 4, and 5. // // DCE 1.1 provides the specification for version 2. // // [1] https://tools.ietf.org/html/rfc4122 // [2] http://pubs.opengroup.org/onlinepubs/9696989899/chap5.htm#tagcjh_08_02_01_01

I originally did this, but found it weird to look at. Now that I've added more content I think it should look better.

theckman · 2018-07-14T03:03:39Z

uuid.go

@@ -67,8 +68,7 @@ var (
 	byteGroups = []int{8, 4, 4, 4, 12}
 )

-// Nil is special form of UUID that is specified to have all
-// 128 bits set to zero.
+// Nil is special form of UUID that is specified to have all 128 bits set to zero.


I would refer back to the name used in the RFC:

Nil is a nil UUID, as specified in RFC-4122, that has all 128 bits set to zero.

theckman · 2018-07-14T03:04:14Z

uuid.go

 func (u UUID) Bytes() []byte {
 	return u[:]
 }

-// Returns canonical string representation of UUID:
+// String returns a canonical string representation of the UUID:


I'd add RFC-4122 after the word canonical to make it more precise.

theckman · 2018-07-14T03:04:57Z

uuid.go

@@ -152,7 +152,7 @@ func (u *UUID) SetVariant(v byte) {
 // Must is a helper that wraps a call to a function returning (UUID, error)
 // and panics if the error is non-nil. It is intended for use in variable
 // initializations such as
-//	var packageUUID = uuid.Must(uuid.FromString("123e4567-e89b-12d3-a456-426655440000"));


;

Hahahaha! Nice find! 😂

theckman

When you have a moment can you please squash all these commits down to one?

If you aren't familiar with that hit me up on Slack.

theckman · 2018-07-14T20:28:43Z

@trevorstarick As a heads-up, this branch still has two commits in it. My preferred preference is to do things as one atomic commit, when making changes like this. Here are the two commits I see:

commit 1fd6936a12354b8194b959e560f584d60b40be80 (HEAD -> trevorstarick-trevorstarick-improving-documentation)
Author: Trevor Starick <trevor.starick@gmail.com>
Date:   Fri Jul 13 23:56:38 2018 -0400

    documentation: amends review requested for pull request #12

commit 5a402ec3394d4f193ca664b68c17f12b62c3e132
Author: Trevor Starick <trevor.starick@gmail.com>
Date:   Fri Jul 13 21:12:41 2018 -0400

    documentation: add further clarification to comments and documentation

It's not a huge deal, so to avoid a continued back-and forth on this PR I'll merge this branch as-is.

…umentation Merge branch 'trevorstarick-trevorstarick-improving-documentation' Signed-off-by: Tim Heckman <t@heckman.io>

trevorstarick changed the title ~~documentation: added further clarification to comments and documentation~~ documentation: add further clarification to comments and documentation Jul 14, 2018

theckman requested changes Jul 14, 2018

View reviewed changes

trevorstarick added 2 commits July 14, 2018 02:16

documentation: add further clarification to comments and documentation

5a402ec

documentation: amends review requested for pull request #12

1fd6936

theckman merged commit 1fd6936 into gofrs:master Jul 14, 2018

theckman added a commit that referenced this pull request Jul 14, 2018

Merge pull request #12 from trevorstarick/trevorstarick-improving-doc…

adb2adb

…umentation Merge branch 'trevorstarick-trevorstarick-improving-documentation' Signed-off-by: Tim Heckman <t@heckman.io>

trevorstarick deleted the trevorstarick-improving-documentation branch July 14, 2018 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

documentation: add further clarification to comments and documentation #12

documentation: add further clarification to comments and documentation #12

trevorstarick commented Jul 14, 2018

acln0 commented Jul 14, 2018

coveralls commented Jul 14, 2018 •

edited

Loading

theckman commented Jul 14, 2018

theckman left a comment

theckman Jul 14, 2018

trevorstarick Jul 14, 2018

theckman Jul 14, 2018

theckman Jul 14, 2018

theckman Jul 14, 2018

theckman Jul 14, 2018

theckman Jul 14, 2018

theckman Jul 14, 2018

trevorstarick Jul 14, 2018

theckman Jul 14, 2018

theckman Jul 14, 2018

theckman Jul 14, 2018

theckman left a comment

theckman commented Jul 14, 2018

documentation: add further clarification to comments and documentation #12

documentation: add further clarification to comments and documentation #12

Conversation

trevorstarick commented Jul 14, 2018

acln0 commented Jul 14, 2018

coveralls commented Jul 14, 2018 • edited Loading

Pull Request Test Coverage Report for Build 33

💛 - Coveralls

theckman commented Jul 14, 2018

theckman left a 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

Choose a reason for hiding this comment

theckman left a comment

Choose a reason for hiding this comment

theckman commented Jul 14, 2018

coveralls commented Jul 14, 2018 •

edited

Loading