-
Notifications
You must be signed in to change notification settings - Fork 69
feat: Allow Event Reading API to pass numeric parameter #1019
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1019 +/- ##
==========================================
- Coverage 44.91% 44.83% -0.08%
==========================================
Files 128 128
Lines 7555 7554 -1
==========================================
- Hits 3393 3387 -6
- Misses 3803 3808 +5
Partials 359 359 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
436d41d to
6523cb5
Compare
Allow Event Reading API to pass numeric parameter Signed-off-by: bruce <[email protected]>
6523cb5 to
9ee158e
Compare
judehung
left a comment
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.
@weichou1229 For tracking purposes, please include a description in the PR noting that you updated the EventClient and ReadingClient interfaces with XXXXWithQueryParams. This was done to maintain backward compatibility, ensuring that other services using these client libraries won’t need to make significant code changes.
dtos/reading.go
Outdated
| switch v := aux.Value.(type) { | ||
| case string: // string, for JSON strings | ||
| b.Value = v | ||
| case float64: // float64, for JSON numbers |
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.
Add more explanation in comment to describe why only deal with float64 here
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.
Updated according to Cloud's comment. The json lib treats all number type as float64, so we don't need to handle other types.
clients/interfaces/event.go
Outdated
| // limit: The number of items to return. Specify -1 will return all remaining items after offset. The maximum will be the MaxResultCount as defined in the configuration of service. Default is 20. | ||
| EventsByDeviceName(ctx context.Context, name string, offset, limit int) (responses.MultiEventsResponse, errors.EdgeX) | ||
| // EventsByDeviceNameWithQueryParams returns a portion of the entire events according to the device name and specified query parameters. Events are sorted in descending order of created time. | ||
| EventsByDeviceNameWithQueryParams(ctx context.Context, name string, queryParams map[string]string) (responses.MultiEventsResponse, errors.EdgeX) |
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.
| EventsByDeviceNameWithQueryParams(ctx context.Context, name string, queryParams map[string]string) (responses.MultiEventsResponse, errors.EdgeX) | |
| EventsByDeviceNameWithQueryParams(ctx context.Context, name string, offset, limit int, queryParams map[string]string) (responses.MultiEventsResponse, errors.EdgeX) |
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.
The offset and limit are part of the query parameter, so I remove offset, limit int from the function argument.
For example, http://0.0.0.0:59880/api/v3/event/device/name/Random-UnsignedInteger-Device?offset=0&limit=3&numeric=true
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.
Added the offset, limit back.
clients/interfaces/event.go
Outdated
| // limit: The number of items to return. Specify -1 will return all remaining items after offset. The maximum will be the MaxResultCount as defined in the configuration of service. Default is 20. | ||
| EventsByTimeRange(ctx context.Context, start, end int64, offset, limit int) (responses.MultiEventsResponse, errors.EdgeX) | ||
| // EventsByTimeRangeWithQueryParams returns events between a given start, end date/time, and specified query parameters. Events are sorted in descending order of created time. | ||
| EventsByTimeRangeWithQueryParams(ctx context.Context, start, end int64, queryParams map[string]string) (responses.MultiEventsResponse, errors.EdgeX) |
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.
| EventsByTimeRangeWithQueryParams(ctx context.Context, start, end int64, queryParams map[string]string) (responses.MultiEventsResponse, errors.EdgeX) | |
| EventsByTimeRangeWithQueryParams(ctx context.Context, start, end int64, offset, limit int, queryParams map[string]string) (responses.MultiEventsResponse, errors.EdgeX) |
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.
The offset and limit are part of the query parameter, so I remove offset, limit int from the function argument.
For example, http://0.0.0.0:59880/api/v3/event/start/1102168089665565200/end/1840986079160530000?offset=2&numeric=true
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.
Added the offset, limit back.
if the value is not string, let the NumericReading contain the equivalent value Signed-off-by: bruce <[email protected]>
judehung
left a comment
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
cloudxxx8
left a comment
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.
generate mock client
Added offset and limit parameters to the Event and Reading client XXXWithQueryParams functions for easier use. Signed-off-by: bruce <[email protected]>
Generated Event and Reading mock client. |
cloudxxx8
left a comment
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
Allow Event Reading API to pass a numeric parameter.
Updated the EventClient and ReadingClient interfaces to add a new function XXXXWithQueryParams. This was done to maintain backward compatibility, ensuring that other services using these client libraries won’t need to make significant code changes.
If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/go-mod-core-contracts/blob/main/.github/Contributing.md
PR Checklist
Please check if your PR fulfills the following requirements:
BREAKING CHANGE:describing the break)Testing Instructions
New Dependency Instructions (If applicable)