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

Conversation

5 participants
@lucascaro
Copy link
Contributor

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

This comment has been minimized.

Copy link

commented Jun 4, 2019

CLA assistant check
All committers have signed the CLA.

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

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

Why use such a strange directory structure?

- uuid
  - tests
    - v4
     - generate.ts
@lucascaro

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

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 added some commits Jul 1, 2019

@lucascaro lucascaro changed the title add uid module supporting v4 add UUID module supporting v4 Jul 1, 2019

@hayd

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

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

Show resolved Hide resolved uuid/v4.ts Outdated

lucascaro added some commits Jul 2, 2019

@lucascaro lucascaro force-pushed the lucascaro:uuid branch from e7e14af to 4453d43 Jul 2, 2019

@ry

ry approved these changes Jul 3, 2019

Copy link
Contributor

left a comment

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

@ry ry merged commit f52b3ec into denoland:master Jul 3, 2019

5 checks passed

denoland.deno_std Build #20190702.4 succeeded
Details
denoland.deno_std (Linux) Linux succeeded
Details
denoland.deno_std (Mac) Mac succeeded
Details
denoland.deno_std (Windows) Windows succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.