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

allow "IN" Expression to have a list or a set as a parameter #11

Merged
merged 6 commits into from
Mar 23, 2022

Conversation

omiz
Copy link
Contributor

@omiz omiz commented Mar 15, 2022

this is because for values like Int64 passing an array to an IN expression is not allowed and requires variadic parameter

this is because for values like Int64 passing an array to an IN expression is not allowed and requires variadic parameter
@omiz omiz requested a review from ftchirou as a code owner March 15, 2022 11:33
Copy link
Owner

@ftchirou ftchirou left a comment

Choose a reason for hiding this comment

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

Hi @omiz! Thank you for your contribution. The changes look good to me. Before I merge them, could you cover them with some unit tests following the pattern here? And maybe add a note in the README?

@omiz
Copy link
Contributor Author

omiz commented Mar 21, 2022

I added the tests and updated the README file as requested to cover the new "IN" functions.
If there is anything else still missing before the merge please let me know so I can fix it

Copy link
Owner

@ftchirou ftchirou left a comment

Choose a reason for hiding this comment

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

Thank you for the updates. The code coverage went down. I added a suggestion to fix that and another suggestion for the README.

PredicateKitTests/OperatorTests.swift Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@omiz
Copy link
Contributor Author

omiz commented Mar 23, 2022

Thank you @ftchirou for your suggestions. I tried to follow them as close as possible to what you mentioned

Copy link
Owner

@ftchirou ftchirou left a comment

Choose a reason for hiding this comment

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

Looking good. 🎉

@ftchirou ftchirou merged commit 64169a9 into ftchirou:main Mar 23, 2022
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