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

Jump to bottom

fix(model.d): fix type for count and findAndCountAll #13786

Merged
merged 3 commits into from Jan 12, 2022
Merged

fix(model.d): fix type for count and findAndCountAll #13786

merged 3 commits into from Jan 12, 2022

Conversation

zorji
Copy link
Contributor

@zorji zorji commented Dec 18, 2021 •
edited

Pull Request Checklist

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

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

Description Of Change

Fix incorrect typing

@zorji zorji mentioned this pull request Dec 18, 2021
Merged
6 tasks
zorji
zorji reviewed Dec 24, 2021
View changes
types/lib/model.d.ts Show resolved Hide resolved
zorji
zorji reviewed Dec 24, 2021
View changes
types/lib/model.d.ts Show resolved Hide resolved
@zorji zorji marked this pull request as ready for review Dec 24, 2021
@zorji zorji changed the title WIP fix(model.d): corrected count type fix(model.d): fix type for count and findAndCountAll Dec 24, 2021
@zorji zorji marked this pull request as draft Dec 25, 2021
@zorji zorji marked this pull request as ready for review Dec 25, 2021
@zorji
Copy link
Contributor Author

@zorji zorji commented Dec 25, 2021

Hi @sdepold, sorry I have to tag you because I am unable who to get help with.

The CI failed with a spec that is marked is "Flaky" for all CI run in MySQL 8.0.

However, I am unable to reproduce the same issue locally. Beside I only changed the types not any implementation and the failed test is related to response data ordering.

I am not sure what should I do next as I don't have permissions to re-run the test and I am unable to reproduce.

Could you please let me know who can I get help with?

@WikiRik WikiRik requested a review from AllAwesome497 Dec 25, 2021
hsource
hsource suggested changes Dec 27, 2021
View changes
Copy link
Contributor

@hsource hsource left a comment

Hi! I'm the original contributor of a lot of the Typescript typings in #12405 - thanks a ton for these improvements! I just had a few small style nits, but otherwise this looks great.

docs/manual/core-concepts/model-querying-finders.md Show resolved Hide resolved
types/lib/model.d.ts Outdated Show resolved Hide resolved
types/test/model.ts Outdated Show resolved Hide resolved
types/test/model.ts Outdated Show resolved Hide resolved
types/test/model-count.ts Outdated Show resolved Hide resolved
@ephys ephys added the type: typescript label Jan 3, 2022
ephys
ephys requested changes Jan 6, 2022
View changes
Copy link
Member

@ephys ephys left a comment •
edited

I didn't notice this PR before sending #13884, So sorry!

I'm still interested in merging yours as it takes findAndCount into account.
Could you resolve the merge conflict? I'll take care of merging this once it's done

Also don't worry about the failing mysql 8 test. We'll merge even if it fails.

types/lib/model.d.ts Outdated Show resolved Hide resolved
types/test/model-count.ts Outdated Show resolved Hide resolved
types/test/model.ts Outdated Show resolved Hide resolved
@ephys ephys self-assigned this Jan 6, 2022
@zorji zorji force-pushed the fix-model-count branch 2 times, most recently from fc0dd95 to 4d99ea8 Compare Jan 7, 2022
@zorji
Copy link
Contributor Author

@zorji zorji commented Jan 7, 2022

Thanks @ephys

I have resolved the conflict, I can see you have made a similar change for grouped count, let me know what else I could help with.

@WikiRik WikiRik requested a review from ephys Jan 10, 2022
@ephys
Copy link
Member

@ephys ephys commented Jan 11, 2022

@zorji the tests are not passing with your changes, could you take a look? :)

@zorji
Copy link
Contributor Author

@zorji zorji commented Jan 12, 2022

@ephys thanks for checking, the build is fixed now.

ephys
ephys approved these changes Jan 12, 2022
View changes
@ephys
Copy link
Member

@ephys ephys commented Jan 12, 2022 •
edited

Thank you! Merging once the CI is done.

@WikiRik WikiRik merged commit cf27c66 into sequelize:main Jan 12, 2022
44 checks passed
@ephys ephys added this to To be released in Sequelize v6 release log Jan 12, 2022
@sdepold sdepold moved this from To be released to Releasing in Sequelize v6 release log Jan 22, 2022
@sdepold sdepold moved this from Releasing to To be released in Sequelize v6 release log Jan 22, 2022
@sdepold sdepold moved this from To be released to Releasing in Sequelize v6 release log Jan 22, 2022
@sdepold sdepold moved this from Releasing to To be released in Sequelize v6 release log Jan 22, 2022
@sdepold sdepold moved this from To be released to Releasing in Sequelize v6 release log Jan 22, 2022
sdepold pushed a commit that referenced this issue Jan 22, 2022
@zorji @ephys @sdepold
fix(model.d): fix type for count and findAndCountAll (#13786)
b06c1fc 
* fix(model): add stop mysql-8 script

* fix(model): fix type for `count` and `findAndCountAll`

Co-authored-by: Zoé <zoe@ephys.dev>
@sdepold sdepold moved this from Releasing to Released in Sequelize v6 release log Jan 23, 2022
@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Feb 7, 2022

🎉 This PR is included in version 7.0.0-alpha.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

aliatsis pushed a commit to creditiq/sequelize that referenced this issue Jun 2, 2022
@zorji @ephys @aliatsis
fix(model.d): fix type for count and findAndCountAll (sequelize#1…
9e5bead 
…3786)

* fix(model): add stop mysql-8 script

* fix(model): fix type for `count` and `findAndCountAll`

Co-authored-by: Zoé <zoe@ephys.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsource hsource

@ephys ephys

@AllAwesome497 AllAwesome497

Assignees

@ephys ephys

Labels
released on @v7 type: typescript
Projects
Sequelize v6 release log
Released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@zorji @ephys @hsource @WikiRik