Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Databroker API V2 #33
Databroker API V2 #33
Changes from 15 commits
75b440f
2e103a6
c01314a
50ca9a4
ce13516
32c79d8
761d9c0
ce7cfc1
3221203
5afc533
233138e
48a603f
982968d
6f12990
70c41c4
5ce44ce
49f2a8f
077d488
d11f942
440b82a
84477a2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@argerus @rafaeling
Why I like the "oneof":
Assuming the ValueFailure represents the reason why a "real" value is missing (maybe we should rename it to "MissingReason"?), the oneof has some advantages over two separate fields (e.g. value and value_state):
I don't see many disadvantages:
Just checking if a value is there and otherwise ignoring the reason why it's missing is as easy as just having a value field or having an additional value_state field. At least in C++:
This C++ code would be the same - regardless if we have:
(1)
or (2)
or (3)
Besides that, I think transporting additional information about a present value (like some quality information (like "outdated" or other things we discussed today), doesn't belong into an information field describing reasons why a value is missing. Those informations aren't related.
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.
Could we call the message signal? For me it was hard to follow during the usage of the SingalID message that this actually either an id or the path.
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 term "signal" would be more than just the identifier of it. Therefore, I like the name "SignalId": "Id" is short for "identifier" which not necessarily has to be a number. It could be anything capable of serving as a unique id - therefore also a string.
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, those can be removed now
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.
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.