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 marked this pull request as ready for review December 24, 2021 04:16
@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 December 25, 2021 05:58
@zorji zorji marked this pull request as ready for review December 25, 2021 06:00
@zorji
Copy link
Contributor Author

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?

hsource
hsource suggested changes Dec 27, 2021
View reviewed changes
Copy link
Contributor

@hsource hsource left a comment

Choose a reason for hiding this comment

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

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 For issues and PRs. Things that involve typescript, such as typings and intellisense. label Jan 3, 2022
ephys
ephys requested changes Jan 6, 2022
View reviewed changes
Copy link
Member

@ephys ephys left a comment •
edited

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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 January 10, 2022 22:13
@ephys
Copy link
Member

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 commented Jan 12, 2022

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

@ephys
Copy link
Member

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
sdepold pushed a commit that referenced this pull request 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>
@github-actions
Copy link
Contributor

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 pull request 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 hsource requested changes

@ephys ephys ephys approved these changes

@wbourne0 wbourne0 Awaiting requested review from wbourne0

Assignees

@ephys ephys

Labels
released on @v7 type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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