Skip to content

Conversation

@danieljbruce
Copy link
Contributor

In the filter function a switch statement is added so that it is clear to the reader what happens when the arguments are of various lengths.

Fixes #1096

In the filter function a switch statement is added so that it is clear to the reader what happens when the arguments are of various lengths
@danieljbruce danieljbruce requested review from a team as code owners May 3, 2023 20:53
@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcf bot commented May 3, 2023

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@product-auto-label product-auto-label bot added size: s Pull request size is small. api: datastore Issues related to the googleapis/nodejs-datastore API. labels May 3, 2023
@danieljbruce danieljbruce changed the title chore: Add a switch statement chore(filter): Add a switch statement May 3, 2023
src/query.ts Outdated
case 1: {
if (isFilter(propertyOrFilter)) {
this.entityFilters.push(propertyOrFilter);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the else branch should be removed here (operatorOrValue and value are not provided)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

src/query.ts Outdated
case 2: {
this.filters.push({
name: (propertyOrFilter as String).trim(),
op: '=' as Operator,
Copy link
Contributor

Choose a reason for hiding this comment

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

as Operator is probably not needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup

Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

LGTM with a couple nits

Remove the else block, it should never happen anyway because if it does then the user is misusing the function
Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

Nice!

@danieljbruce danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label May 29, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 29, 2023
@danieljbruce danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label May 31, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 31, 2023
@danieljbruce danieljbruce merged commit 82fab3d into googleapis:main Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: datastore Issues related to the googleapis/nodejs-datastore API. size: s Pull request size is small.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change filter in query implementation to use a switch

3 participants