-
Notifications
You must be signed in to change notification settings - Fork 18
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
Remove oneof and ValueFailure from Datapoint and some proto naming improvements #72
Remove oneof and ValueFailure from Datapoint and some proto naming improvements #72
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/databroker-api-v2 #72 +/- ##
============================================================
Coverage ? 51.75%
============================================================
Files ? 33
Lines ? 12893
Branches ? 0
============================================================
Hits ? 6673
Misses ? 6220
Partials ? 0 ☔ View full report in Codecov by Sentry. |
proto/kuksa/val/v2/types.proto
Outdated
ValueFailure failure = 2; | ||
Value value = 3; | ||
} | ||
Value value = 2; |
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.
So what is now supposed to be returned if there is no value available, a value
that is None
?
Like if you do a get request, but no value available yet, should yet get a Datapoint back where value is None. If so, maybe we shall add a comment that value may be None to indicate no value available (but signal known)
Well, actually I see no need for change here if we agree that value
may be None in some scenarios, then I can add a comment after rebasing my PR.
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 accept this change, but comment as proposed by Erik should be added.
Remark:
If needed we could later re-add the oneof (with renamed fields) and still stay compatible with the current version:
message Datapoint {
google.protobuf.Timestamp timestamp = 1;
oneof value_state {
Value value = 2;
MissingReason missingValueReason = 3;
}
}
enum MissingReason {
...
}
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.
@rafaeling - will you update so we can have it merged (If my interpretation of when None shall be used is correct)
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 accept this change, but comment as proposed by Erik should be added. Remark: If needed we could later re-add the oneof (with renamed fields) and still stay compatible with the current version:
message Datapoint { google.protobuf.Timestamp timestamp = 1; oneof value_state { Value value = 2; MissingReason missingValueReason = 3; } } enum MissingReason { ... }
I added it also to the discussion topic list - https://github.com/eclipse-kuksa/kuksa-databroker/pull/21/files#diff-7ed9bb015da6a2a5d42536ce062cb6df04a6b8a9688a4dcfe15995a3ef300087R556
maybe if we use one day the design documentation we could also add the discussion link why it was decided to design the proto message that way.
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.
@rafaeling - will you update so we can have it merged (If my interpretation of when None shall be used is correct)
Will update it
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.
Proto files lgtm
proto/kuksa/val/v2/types.proto
Outdated
ValueFailure failure = 2; | ||
Value value = 3; | ||
} | ||
Value value = 2; |
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 accept this change, but comment as proposed by Erik should be added.
Remark:
If needed we could later re-add the oneof (with renamed fields) and still stay compatible with the current version:
message Datapoint {
google.protobuf.Timestamp timestamp = 1;
oneof value_state {
Value value = 2;
MissingReason missingValueReason = 3;
}
}
enum MissingReason {
...
}
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
4eee61c
into
eclipse-kuksa:feature/databroker-api-v2
…provements (#72) * Remove oneof from Datapoint
…provements (#72) * Remove oneof from Datapoint
…provements (#72) * Remove oneof from Datapoint
…provements (#72) * Remove oneof from Datapoint
…provements (#72) * Remove oneof from Datapoint
No description provided.