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 support for all UUID versions #92

Merged
merged 4 commits into from
Aug 15, 2017

Conversation

IzikLisbon
Copy link
Contributor

@IzikLisbon IzikLisbon commented Aug 10, 2017

Sequelize is using Validator to validates UUID strings.
Validator supports 4 versions of UUID - 3,4,5 and all:
https://github.com/chriso/validator.js/blob/b59133b1727b6af355b403a9a97a19226cceb34b/lib/isUUID.js#L14-L19

This change is updating IsUUID to support all possible options - 3,4,5 and all.

see DefinitelyTyped/DefinitelyTyped#18777 (comment)

Other stuff
Two additional changes:

  1. WhereOptions --> WhereOptions<any>
  2. added as IAssociationOptionsBelongsToMany

I am not sure about any of those additional changes. I know it passed compilation and testing, but I basically choose the option that compiles.

Also running npm run build before making any change failed. I have upgraded to node 8 and then it passed. Is there any dependency on the node version or it was just something locally on my machine?

2. Running the tests failed with an error message asking to install sqlite3 manually, so I did and it worked. When I installed manually it installed version 3.1.9, since it worked I kept the upgraded version.
@codecov-io
Copy link

Codecov Report

Merging #92 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #92   +/-   ##
=======================================
  Coverage   95.65%   95.65%           
=======================================
  Files          66       66           
  Lines         759      759           
  Branches      106      106           
=======================================
  Hits          726      726           
  Misses         11       11           
  Partials       22       22
Impacted Files Coverage Δ
lib/annotations/association/BelongsToMany.ts 100% <100%> (ø) ⬆️
lib/annotations/validation/IsUUID.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84a0538...a24fd7c. Read the comment docs.

@RobinBuschmann
Copy link
Member

Looks good, thank you for implementing that.

One question: You've updated @types/sequelize to 4.0.68. Was this a required step to achieve this feature?

@IzikLisbon
Copy link
Contributor Author

IzikLisbon commented Aug 11, 2017

@IzikLisbon
Copy link
Contributor Author

@RobinBuschmann build and tests have failed on my machine. I tried the following

  1. clone
  2. npm install
  3. npm run build

Got a failure (Don't remember what it was).
Upgraded to node8, ran again. This time it was successful. Is there any dependency on the node version?

Then I have tried to run:
npm run cover

got an error, asking to install sqlite3 manually. So I ran:

  1. npm install sqlite3
  2. npm run cover

This time it worked.

I can open an issue for this, but wanna know if you have any ideas.

@IzikLisbon
Copy link
Contributor Author

IzikLisbon commented Aug 14, 2017

@RobinBuschmann anything that is blocking this merge?

@RobinBuschmann RobinBuschmann merged commit 61604c8 into sequelize:master Aug 15, 2017
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.

3 participants