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

MF-1314 - Add value comparison filters for readers #1353

Merged
merged 12 commits into from
Feb 9, 2021
Merged

MF-1314 - Add value comparison filters for readers #1353

merged 12 commits into from
Feb 9, 2021

Conversation

manuio
Copy link
Contributor

@manuio manuio commented Feb 4, 2021

Signed-off-by: Manuel Imperiale manuel.imperiale@gmail.com

Resolves #1314

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio manuio requested a review from a team as a code owner February 4, 2021 10:51
switch val.(string) {
case "equal":
comparison = "="
case "lower-than":
Copy link
Contributor

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?

Copy link
Collaborator

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.

Copy link
Contributor

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

Offset: 0,
Limit: limit,
Value: v + 1,
Comparison: "lte",
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 we mentioned le instead of lte, according to ARM assembly spec.

@dusanb94 @nmarcetic opinions?

@codecov-io
Copy link

Codecov Report

Merging #1353 (f31bf6e) into master (13c426c) will decrease coverage by 0.00%.
The diff coverage is 76.66%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
readers/api/transport.go 79.19% <50.00%> (-0.81%) ⬇️
readers/cassandra/messages.go 58.88% <50.00%> (-1.34%) ⬇️
readers/postgres/messages.go 62.36% <70.00%> (-2.47%) ⬇️
readers/influxdb/messages.go 75.18% <75.00%> (-1.15%) ⬇️
readers/mongodb/messages.go 78.08% <90.00%> (+2.22%) ⬆️
readers/api/requests.go 100.00% <100.00%> (ø)
things/service.go 53.40% <0.00%> (-1.14%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13c426c...f31bf6e. Read the comment docs.

enum:
- eq
- lt
- lte
Copy link
Collaborator

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.

Copy link
Contributor Author

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 <manuel.imperiale@gmail.com>
dborovcanin
dborovcanin previously approved these changes Feb 9, 2021
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

@drasko drasko merged commit f0f60e2 into absmach:master Feb 9, 2021
fbugarski pushed a commit to fbugarski/mainflux that referenced this pull request Mar 8, 2021
* MF-1314 - Add value comparison filters for readers

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Check if comparison parameter is valid

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Use eq, lt, lte, gt, gte as comparison operator keys

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Use consts for comparison operators

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Use comparator naming instead of comparison

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Fix openapi.yml

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Fix typo

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio manuio deleted the value branch February 23, 2022 12:38
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.

Consider adding additional filters by SenML value
4 participants