Skip to content

Docs and more tests for #9087 #10312

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.

Sign up for GitHub

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

Jump to bottom
Merged
sushantdhiman merged 2 commits into from
Jan 3, 2019
Merged

Docs and more tests for #9087 #10312

sushantdhiman merged 2 commits into from
Jan 3, 2019

Conversation

@papb
Copy link
Member

@papb papb commented Jan 3, 2019 •
edited
Loading

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

The PR #9735 solved the issue #9087 very nicely. I was thinking that it deserved more tests, to help preventing regressions, and also deserved one section of the tutorial docs to be better explained.

This way, in this PR I have added a strong test to prevent regressions, and also some extra documentation.

@papb
Copy link
Member Author

papb commented Jan 3, 2019

Note: for the tests I used a library called js-combinatorics, and for that I added it to package.json. According to packagephobia it is only 62 kB, so I think it should be fine. It is very useful to quickly assert that the order in which the merge is applied does not matter (which is something I wanted my test to check).

If adding extra dev-dependencies like this is not good, I can hard-code all the permutations directly, but that would be a bit ugly and unreadable (in my opinion).

@codecov
Copy link

codecov bot commented Jan 3, 2019 •
edited
Loading

Codecov Report

Merging #10312 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10312   +/-   ##
=======================================
  Coverage   96.28%   96.28%           
=======================================
  Files          68       68           
  Lines        9772     9772           
=======================================
  Hits         9409     9409           
  Misses        363      363

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 ecb0868...352c708. Read the comment docs.

@sushantdhiman
Copy link
Contributor

sushantdhiman commented Jan 3, 2019

Thanks @papb 👍

@sushantdhiman sushantdhiman merged commit 2a4e111 into sequelize:master Jan 3, 2019
@papb papb mentioned this pull request Jan 17, 2019
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@sushantdhiman sushantdhiman sushantdhiman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@papb @sushantdhiman