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

add UUID module supporting v4 #479

Merged
merged 9 commits into from
Jul 3, 2019
Merged

add UUID module supporting v4 #479

merged 9 commits into from
Jul 3, 2019

Conversation

lucascaro
Copy link
Contributor

@lucascaro lucascaro commented Jun 4, 2019

Fixes #403

  • add generation and validation of uuid v4
  • tests for v4
  • uuid v1
  • uuid v3
  • uuid v5
  • use crypto. getRandomValues()

@CLAassistant
Copy link

CLAassistant commented Jun 4, 2019

CLA assistant check
All committers have signed the CLA.

uuid/test.ts Outdated Show resolved Hide resolved
uuid/tests/generate.ts Outdated Show resolved Hide resolved
@axetroy
Copy link
Contributor

axetroy commented Jul 1, 2019

Why use such a strange directory structure?

- uuid
  - tests
    - v4
     - generate.ts

@lucascaro
Copy link
Contributor Author

Why use such a strange directory structure?

- uuid
  - tests
    - v4
     - generate.ts

I added /tests/ because I prefer to keep all test files separated from the code.

v4 in there is to be prepared to add tests for v1, v3, and v5 while keeping them clearly separated.

generate vs validate is personal preference too: test files that deal with a single concern or topic.

I'm totally open to suggestions if this doesn't follo deno_std best practices

@lucascaro lucascaro changed the title add uid module supporting v4 add UUID module supporting v4 Jul 1, 2019
@hayd
Copy link
Contributor

hayd commented Jul 1, 2019

Just to confirm, this is based of this gist? https://gist.github.com/jed/982883#gistcomment-2886092

Is that not a concern? (I am a little confused whether this was code golf.)

@lucascaro
Copy link
Contributor Author

@hayd the code in my module was originally based off a version in that gist. The current iteration looks different.
I think at this point it's important to focus on the api this is exposing rather than the specific implementation since the implementation can change any time, but if this makes it into deno_std, other modules will depend on it maintaining a compatible api.

uuid/v4.ts Outdated Show resolved Hide resolved
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @lucascaro ! I think we can iterate further in the repo

@ry ry merged commit f52b3ec into denoland:master Jul 3, 2019
ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019
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.

import https://github.com/lucascaro/deno-uuid into deno_std
5 participants