Skip to content

fix(types): relax order typing#10802

Merged
sushantdhiman merged 1 commit intosequelize:masterfrom
Chocobozzz:feature/typing-order
Jul 6, 2019
Merged

fix(types): relax order typing#10802
sushantdhiman merged 1 commit intosequelize:masterfrom
Chocobozzz:feature/typing-order

Conversation

@Chocobozzz
Copy link
Copy Markdown
Contributor

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? yes
  • Have you added new tests to prevent regressions? no
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)? no
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Relax type order checking in queries, because we can have multiple nester models and or strings etc. I'm not sure we can do better than this.

Closes #10780

@eseliger
Copy link
Copy Markdown
Member

hmm now [string, typeof Model] is also allowed..

Maybe we define explicitly up to 5 or 10 levels? Not perfect either but prevents wrong usage as shown above 🤔

Copy link
Copy Markdown
Contributor

@SimonSchick SimonSchick left a comment

Choose a reason for hiding this comment

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

Missing tests.

@SimonSchick SimonSchick added pr:needs test typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. labels Apr 20, 2019
@Chocobozzz Chocobozzz force-pushed the feature/typing-order branch from 947be6f to c339659 Compare April 23, 2019 09:19
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 23, 2019

Codecov Report

Merging #10802 into master will increase coverage by 21.81%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #10802       +/-   ##
===========================================
+ Coverage   74.54%   96.35%   +21.81%     
===========================================
  Files          85       94        +9     
  Lines        8186     9008      +822     
===========================================
+ Hits         6102     8680     +2578     
+ Misses       2084      328     -1756
Impacted Files Coverage Δ
lib/dialects/mariadb/connection-manager.js 100% <0%> (ø)
lib/dialects/mariadb/index.js 100% <0%> (ø)
lib/dialects/sqlite/index.js 100% <0%> (ø)
lib/dialects/mariadb/query.js 87.69% <0%> (ø)
lib/dialects/postgres/connection-manager.js 95.77% <0%> (ø)
lib/dialects/postgres/index.js 100% <0%> (ø)
lib/dialects/sqlite/query.js 98.64% <0%> (ø)
lib/dialects/sqlite/connection-manager.js 100% <0%> (ø)
lib/dialects/postgres/query.js 97.68% <0%> (ø)
lib/model.js 96.68% <0%> (+0.86%) ⬆️
... and 29 more

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 30c5ca5...fd56e99. Read the comment docs.

@Chocobozzz Chocobozzz force-pushed the feature/typing-order branch from c339659 to b6186b5 Compare April 23, 2019 09:20
@Chocobozzz
Copy link
Copy Markdown
Contributor Author

@eseliger Please take another look

@SimonSchick I added tests

@eseliger
Copy link
Copy Markdown
Member

eseliger commented May 4, 2019

needs rebase

@Chocobozzz Chocobozzz force-pushed the feature/typing-order branch from ca97322 to fd56e99 Compare May 9, 2019 09:27
@Chocobozzz
Copy link
Copy Markdown
Contributor Author

@eseliger done

@gibsonjoshua55
Copy link
Copy Markdown

gibsonjoshua55 commented Jun 28, 2019

When can we expect this change to be merged?

@sushantdhiman sushantdhiman merged commit 75b95dc into sequelize:master Jul 6, 2019
@sushantdhiman
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 5.9.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

schmod pushed a commit to schmod/sequelize that referenced this pull request Jul 10, 2019
…ndle-deadlock

* 'master' of github.com:sequelize/sequelize: (22 commits)
  docs(migrations): use timestamps with seed (sequelize#11160)
  test: remove redundant test (sequelize#11156)
  fix(types): add literal to possible where options (sequelize#10990)
  fix(model): don't alter original scopes when combining them (sequelize#10722)
  fix(types): relax order typing (sequelize#10802)
  fix(types): add string to Includeable (sequelize#11003)
  docs(models-definition): correct spelling mistakes (sequelize#11147)
  fix(types): silent option for update (sequelize#11115)
  fix: update sequelize-pool (sequelize#11134)
  feat(hooks): beforeDisconnect / afterDisconnect (sequelize#11117)
  refactor: remove unused _templateSettings
  refactor(query-generation): remove lodash string templates (sequelize#11122)
  docs: improve datatype docs
  docs: explain defaults/where behavior for find/create (sequelize#11069)
  build: remove test*.js from .gitignore (sequelize#11108)
  docs(data-types): extending types
  fix(sequelize.close): update sequelize-pool (sequelize#11101)
  build: update dependencies (sequelize#11099)
  docs(migrations): foreign key example (sequelize#11097)
  fix(mariadb): properly escape json path key (sequelize#11089)
  ...
@sushantdhiman
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 7.0.0-next.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can only order 2 models deep

5 participants