Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ULID.generate/0, ULID.generate/1 and ULID.decode/1 #1

Merged
merged 12 commits into from
Mar 29, 2018

Conversation

unnawut
Copy link
Contributor

@unnawut unnawut commented Mar 26, 2018

Issue/Task Number: T147

Overview

Implements the main logic for ULID.generate/0, ULID.generate_at/1 and ULID.decode/1.

Changes

  • Implemented the Crockford base32 encoder module
  • Implemented logic for ULID.generate/0
  • Implemented logic for ULID.generate_at/1
  • Implemented logic for ULID.decode/1
  • Added benchmarking

Implementation Details

N/A

Usage

Try the instructions provided in the README.md

Impact

N/A

@unnawut unnawut added the enhancement 🚀 New feature or request label Mar 26, 2018
@unnawut unnawut changed the title Implement ULID.generate and ULID.decode Implement ULID.generate/0, ULID.generate_at/1 and ULID.decode/1 Mar 26, 2018
def decode(ulid) when byte_size(ulid) != 26 do
{:error, "the ULID must be 26 characters long, got #{inspect(ulid)}"}
end
def decode(ulid) do
Copy link

Choose a reason for hiding this comment

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

def decode(<<time::bytes-size(10), id::bytes-size(16)>>)

@doc """
Generates a ULID.
"""
def generate do
Copy link

Choose a reason for hiding this comment

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

Let's have generate/0 and generate/1 instead of generate_at/1

def generate do
  :milli_seconds
  |> :os.system_time()
  |> generate()
end

def generate(time) do
   # ...
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that there might be generate(opts) in the future and this won't work:

def generate(opts \\ [])
def generate(time, opts \\ [])

Thoughts?

Copy link

Choose a reason for hiding this comment

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

Let's not code for something that won't come :-)

def generate_at(time) do
rand = :crypto.strong_rand_bytes(10) # 10 bytes = 80 bits
encode_time(time) <> encode(rand, 16)
rescue
Copy link

Choose a reason for hiding this comment

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

I not sure about the rescue pattern (in Erlang we almost never do this) because it obscure where the error can happen in the stack. Can we instead return {:error, "message"} directly?

Copy link

Choose a reason for hiding this comment

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

I think maybe something like:

case encode(time, 10) do
  {:error, error} -> {:error, error}
  encoded_time ->
    rand = :crypto.strong_rand_bytes(10)
    case encode(rand, 16) do
      {:error, error} -> {:error, error}
      encoded_rand -> encoded_time <> encoded_rand
    end
end

It should make it clear where can the error raise.

end

defp encode_time(time) when not is_integer(time) do
raise InvalidTimeError, message: "time must be an integer, got #{inspect(time)}"
Copy link

Choose a reason for hiding this comment

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

Following the above comment, I think this is something better handled in generate/1 when time is passed, rather than deep down in the stack (whereas encode itself doesn't care about max_time and I think it shouldn't be aware of)

@unnawut
Copy link
Contributor Author

unnawut commented Mar 27, 2018

@sirn Please review again

P.S. I need your help setting up CI for this one. There's probaly more to do than just copy & edit the Jenkinsfile.

Copy link

@sirn sirn left a comment

Choose a reason for hiding this comment

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

Awesome! 🎉

@unnawut unnawut changed the title Implement ULID.generate/0, ULID.generate_at/1 and ULID.decode/1 Implement ULID.generate/0, ULID.generate/1 and ULID.decode/1 Mar 29, 2018
@unnawut unnawut merged commit cd1f52d into master Mar 29, 2018
@unnawut unnawut deleted the dev-ulid branch March 29, 2018 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🚀 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants