-
Notifications
You must be signed in to change notification settings - Fork 671
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
MF-1314 - Add value comparison filters for readers #1353
Conversation
Signed-off-by: Manuel Imperiale <[email protected]>
readers/messages.go
Outdated
switch val.(string) { | ||
case "equal": | ||
comparison = "=" | ||
case "lower-than": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question - do we need lower-than
, or lower
and lower-equal
(or maybe lower-or-equal
) would work?
@dusanb94 @nmarcetic @mteodor @darkodraskovic opinions please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe lt
, gt
, le
, ge
. While it sounds like abbreviations, it is common terminology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can borrow the naming from assembly conditional instructions: https://developer.arm.com/documentation/dui0068/b/arm-instruction-reference/conditional-execution
Signed-off-by: Manuel Imperiale <[email protected]>
Signed-off-by: Manuel Imperiale <[email protected]>
readers/cassandra/messages_test.go
Outdated
Offset: 0, | ||
Limit: limit, | ||
Value: v + 1, | ||
Comparison: "lte", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we mentioned le
instead of lte
, according to ARM assembly spec.
@dusanb94 @nmarcetic opinions?
Codecov Report
@@ Coverage Diff @@
## master #1353 +/- ##
==========================================
- Coverage 60.56% 60.56% -0.01%
==========================================
Files 123 123
Lines 9314 9346 +32
==========================================
+ Hits 5641 5660 +19
- Misses 3188 3196 +8
- Partials 485 490 +5
Continue to review full report at Codecov.
|
Signed-off-by: Manuel Imperiale <[email protected]>
Signed-off-by: Manuel Imperiale <[email protected]>
readers/openapi.yml
Outdated
enum: | ||
- eq | ||
- lt | ||
- lte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's le
and ge
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, thank you!
Signed-off-by: Manuel Imperiale <[email protected]>
Signed-off-by: Manuel Imperiale <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* MF-1314 - Add value comparison filters for readers Signed-off-by: Manuel Imperiale <[email protected]> * Check if comparison parameter is valid Signed-off-by: Manuel Imperiale <[email protected]> * Use eq, lt, lte, gt, gte as comparison operator keys Signed-off-by: Manuel Imperiale <[email protected]> * Use consts for comparison operators Signed-off-by: Manuel Imperiale <[email protected]> * Use comparator naming instead of comparison Signed-off-by: Manuel Imperiale <[email protected]> * Fix openapi.yml Signed-off-by: Manuel Imperiale <[email protected]> * Fix typo Signed-off-by: Manuel Imperiale <[email protected]>
Signed-off-by: Manuel Imperiale [email protected]
Resolves #1314