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

Adding Exclude property to DoubleRangeFilter and test coverage. #1268

Conversation

HazWard
Copy link
Collaborator

@HazWard HazWard commented Oct 15, 2020

What this PR does / Why we need it:
This set of changes gives more control over the excluded/included bounds of DoubleRangeFilters.

Which issue(s) this PR fixes:

Closes #1260

Special notes for your reviewer:

@google-cla google-cla bot added the cla: yes label Oct 15, 2020
@google-cla
Copy link

google-cla bot commented Oct 15, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Oct 15, 2020
@adumovic
Copy link

@googlebot I fixed it

@google-cla
Copy link

google-cla bot commented Oct 15, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@Laremere Laremere self-requested a review October 15, 2020 18:25
@Laremere
Copy link

PR mostly looks good, I would still prefer bools over having the enum, is there more reasoning behind choosing the enum? See comments in: #1260

@Laremere
Copy link

CLA issue is because of the email HazWard@users.noreply.github.com

@Laremere
Copy link

Git amend the PR with your Unity email to fix, I think. (and if possible, set that as your default github email would avoid this problem, I think?)

@HazWard HazWard force-pushed the feature/ConfigurableExclusiveRanges branch from 6d35482 to 8b5eda1 Compare October 15, 2020 18:48
@google-cla google-cla bot added cla: yes and removed cla: no labels Oct 15, 2020
@HazWard
Copy link
Collaborator Author

HazWard commented Oct 15, 2020

Thanks for the tip, I did the merge from the website so it didn't use my work email.

@HazWard
Copy link
Collaborator Author

HazWard commented Oct 15, 2020

We went with enums because we use them often and it gave us a single fields to indicate which bounds should be excluded.

api/messages.proto Show resolved Hide resolved
api/messages.proto Show resolved Hide resolved
internal/filter/filter.go Show resolved Hide resolved
@HazWard HazWard requested a review from Laremere October 21, 2020 17:32
Copy link

@Laremere Laremere left a comment

Choose a reason for hiding this comment

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

I still don't see full test coverage - given that you've moved and (mostly) copied 1 line of logic behind a switch statement with 4 clauses, full coverage implies there to be roughly 4 times the number of tests. I've thought about it more, and suggest grouping by the case, eg:

`
simpleDoubleRange("minIsNan", 5, math.NaN(), 10, pb.DoubleRangeFilter_NONE),
simpleDoubleRange("minIsNan", 5, math.NaN(), 10, pb.DoubleRangeFilter_MIN),
simpleDoubleRange("minIsNan", 5, math.NaN(), 10, pb.DoubleRangeFilter_MAX),
simpleDoubleRange("minIsNan", 5, math.NaN(), 10, pb.DoubleRangeFilter_BOTH),

simpleDoubleRange("maxIsNan", 5, 0, math.NaN(), pb.DoubleRangeFilter_NONE),
simpleDoubleRange("maxIsNan", 5, 0, math.NaN(), pb.DoubleRangeFilter_MIN),
simpleDoubleRange("maxIsNan", 5, 0, math.NaN(), pb.DoubleRangeFilter_MAX),
simpleDoubleRange("maxIsNan", 5, 0, math.NaN(), pb.DoubleRangeFilter_BOTH),

`
(empty line between the groupings)

The cases which are affected by the enum would become obvious, as there won't be full groupings. The names don't strictly need to be different, though it'd probably be helpful for the cases which are different.

api/messages.proto Show resolved Hide resolved
api/messages.proto Outdated Show resolved Hide resolved
@Laremere Laremere merged commit 2eb2921 into googleforgames:master Nov 2, 2020
@syntxerror syntxerror added this to the v1.1.0 milestone Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow excluding min/max in DoubleRangeFilter
4 participants