Skip to content

Adding Bytes() to get slice representation of ULID#49

Merged
peterbourgon merged 3 commits intooklog:masterfrom
sabify:patch-1
May 31, 2019
Merged

Adding Bytes() to get slice representation of ULID#49
peterbourgon merged 3 commits intooklog:masterfrom
sabify:patch-1

Conversation

@sabify
Copy link
Copy Markdown
Contributor

@sabify sabify commented May 29, 2019

No description provided.

@coveralls
Copy link
Copy Markdown

coveralls commented May 29, 2019

Coverage Status

Coverage increased (+0.02%) to 97.959% when pulling 8d45742 on sabify:patch-1 into be3bccf on oklog:master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.0%) to 96.939% when pulling ff438f2 on sabify:patch-1 into be3bccf on oklog:master.

@peterbourgon
Copy link
Copy Markdown
Member

@sabify As silly as it might sound, can you please add a test? 😇 It would be good for the test to make sure the returned slice is distinct from the original ULID array in the ways that are important.

ulid_test.go Outdated
id := ulid.MustNew(ulid.Timestamp(tt), entropy)
bid := id.Bytes()
bid[len(bid)-1]++
if bytes.Compare(id.Bytes(), bid) == 0 {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

S1004: should use bytes.Equal(id.Bytes(), bid) instead (from gosimple)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤗

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

😉

@peterbourgon
Copy link
Copy Markdown
Member

Thanks!

@peterbourgon peterbourgon merged commit 0d4fda9 into oklog:master May 31, 2019
@sabify sabify deleted the patch-1 branch May 31, 2019 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants