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

Finish refactoring queries using new query builder makeQueries #654

Merged
merged 8 commits into from
Feb 5, 2021

Conversation

MattAgn
Copy link
Collaborator

@MattAgn MattAgn commented Jan 15, 2021

Summary

This PR aims to solve this issue. It is the follow up of this PR

Tasks:

  • group queries by their predicate type (byTestId, byText) instead of grouping them by their return
    type (different for get, find, query).
  • maybe group the tests by predicate type instead of query type?

Requirements

  • The API should not change

Test plan

No tests should be added since the API does not change, they only need to be moved to be grouped by predicate type

@MattAgn
Copy link
Collaborator Author

MattAgn commented Jan 15, 2021

@thymikee @mdjastrzebski I modified a bit the error messages to unify them with the errors messages for byTestId which were modified on the previous PR, is it ok for you?

@MattAgn MattAgn force-pushed the refactor/build-queries-for-all branch from c8c59b8 to 9198eb3 Compare January 15, 2021 22:09
@MattAgn
Copy link
Collaborator Author

MattAgn commented Jan 15, 2021

I did not tackle the UNSAFE and a11y queries (since they have their own builder already), is it alright?

@MattAgn MattAgn force-pushed the refactor/build-queries-for-all branch from 9198eb3 to 0895b3f Compare January 23, 2021 13:42
@thymikee thymikee force-pushed the refactor/build-queries-for-all branch from 0895b3f to 8628435 Compare February 5, 2021 13:49
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

This is great, thank you! 🚀

@thymikee thymikee merged commit 34fc03a into callstack:master Feb 5, 2021
@MattAgn
Copy link
Collaborator Author

MattAgn commented Feb 6, 2021

thanks for the approval @thymikee! Should I group the tests by query in a new PR now? Because for now it is done for only one query (byTestId) and not the others

@thymikee
Copy link
Member

thymikee commented Feb 6, 2021

Sure, would be amazing to have the tests cleaned up as well :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants