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

fix(typings): add missing nested to Includeable #11354

Merged
merged 2 commits into from Oct 28, 2019

Conversation

@vanthome
Copy link
Contributor

vanthome commented Aug 25, 2019

Typings were missing previously.

Description of change

Add nested to typings Includeable.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Aug 25, 2019

Codecov Report

Merging #11354 into master will decrease coverage by 8.71%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11354      +/-   ##
==========================================
- Coverage   96.26%   87.55%   -8.72%     
==========================================
  Files          94       91       -3     
  Lines        9190     8847     -343     
==========================================
- Hits         8847     7746    -1101     
- Misses        343     1101     +758
Impacted Files Coverage Δ
lib/dialects/postgres/query-generator.js 1.79% <0%> (-92.59%) ⬇️
lib/dialects/postgres/range.js 7.69% <0%> (-92.31%) ⬇️
lib/dialects/postgres/query-interface.js 7.57% <0%> (-90.91%) ⬇️
lib/errors/database/exclusion-constraint-error.js 18.18% <0%> (-81.82%) ⬇️
lib/dialects/postgres/data-types.js 19.62% <0%> (-76.64%) ⬇️
lib/dialects/postgres/hstore.js 42.85% <0%> (-57.15%) ⬇️
lib/deferrable.js 14.28% <0%> (-57.15%) ⬇️
...dialects/abstract/query-generator/helpers/quote.js 73.33% <0%> (-26.67%) ⬇️
lib/query-interface.js 82.43% <0%> (-9.76%) ⬇️
lib/data-types.js 86.5% <0%> (-4.85%) ⬇️
... and 9 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 c77c553...e90249e. Read the comment docs.

@papb

This comment has been minimized.

Copy link
Member

papb commented Aug 25, 2019

LGTM

@sushantdhiman

This comment has been minimized.

Copy link
Member

sushantdhiman commented Aug 28, 2019

This needs a typing test similar to

MyModel.findAll({

@papb

This comment has been minimized.

Copy link
Member

papb commented Oct 16, 2019

@vanthome it seems you pushed a new commit by accident, am I right?

@vanthome

This comment has been minimized.

Copy link
Contributor Author

vanthome commented Oct 17, 2019

OMG, I did not know that pushes are still synchronized here because I did not crate another PR. I guess when we close this PR this will no longer be the case, or?
Btw. I will prepare this as a fresh PR hopefully soon.

@papb

This comment has been minimized.

Copy link
Member

papb commented Oct 17, 2019

image

Whenever you push to the missing_typing branch, this PR will be updated (unless it was already merged, of course). I am not sure what would happen if the PR is closed (not merged), but I'd guess it would still be updated, except that probably the automatic checks would not run...

If you want to work on multiple PRs at the same time you just have to create one branch for each one and pay attention where you are commiting :)

@papb

This comment has been minimized.

Copy link
Member

papb commented Oct 17, 2019

This needs a typing test similar to

MyModel.findAll({

By the way can you address this comment so we can merge this PR?

@vanthome vanthome force-pushed the vanthome:missing_typing branch from 7d14ae8 to 52eb7ad Oct 18, 2019
@vanthome

This comment has been minimized.

Copy link
Contributor Author

vanthome commented Oct 18, 2019

Ok, I was accidental in a branch. I reverted and rebased. Btw. why is this not merged yet? Is there something missing?

@sushantdhiman

This comment has been minimized.

Copy link
Member

sushantdhiman commented Oct 18, 2019

@papb

This comment has been minimized.

Copy link
Member

papb commented Oct 26, 2019

I think you pushed by mistake again 😬

By the way please address the comment so we can merge this

@vanthome vanthome force-pushed the vanthome:missing_typing branch from f47d0ab to 1af3945 Oct 26, 2019
@papb

This comment has been minimized.

Copy link
Member

papb commented Oct 26, 2019

@vanthome If you need help to create the test let me know

Typings were missing previously.
@vanthome vanthome force-pushed the vanthome:missing_typing branch 2 times, most recently from bc51ea2 to 2f0ddc6 Oct 27, 2019
@vanthome

This comment has been minimized.

Copy link
Contributor Author

vanthome commented Oct 27, 2019

NP, added the test, now please merge it.

@vanthome

This comment has been minimized.

Copy link
Contributor Author

vanthome commented Oct 27, 2019

Just one question. I'm working on another PR but now with the 6.0 master the existing PG tests are failing for me:

  1) [POSTGRES] Model
       schemas
         regressions
           should be able to sync model with schema:
     TypeError: s.replace is not a function
      at Object.removeTicks (lib/utils.js:354:12)
      at Object.quoteIdentifier (lib/dialects/abstract/query-generator/helpers/quote.js:50:35)
      at PostgresQueryGenerator.quoteIdentifier (lib/dialects/abstract/query-generator.js:859:24)
      at PostgresQueryGenerator.quoteTable (lib/dialects/abstract/query-generator.js:906:23)
      at PostgresQueryGenerator.addIndexQuery (lib/dialects/abstract/query-generator.js:543:24)
      at QueryInterface.addIndex (lib/query-interface.js:658:37)
      at /P/BMS-LSA/sequelize/lib/model.js:1358:67
  From previous event:
      at /P/BMS-LSA/sequelize/lib/model.js:1358:24
  From previous event:
      at Function.sync (lib/model.js:1345:8)
      at Context.<anonymous> (test/integration/model/schema.test.js:543:23)
      at processImmediate (internal/timers.js:439:21)

Can you confirm that they are not functioning right now?

@papb
papb approved these changes Oct 27, 2019
Copy link
Member

papb left a comment

Great!

@papb

This comment has been minimized.

Copy link
Member

papb commented Oct 27, 2019

About the tests you mentioned, that's odd, latest commit on master has a green checkmark... I suggest you proceed with your PR, if you think the error is unrelated just ignore it for now, we shall look into it once you open the PR :)

@sushantdhiman sushantdhiman merged commit 0201889 into sequelize:master Oct 28, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.