Add parametric data proto file #82
Conversation
There was a problem hiding this comment.
Please add comments to messages and fields as needed. I would say comments for the messages explaining their purpose is probably the minimum. If the a field comment provides no value, then I'm OK omitting it.
There is inconsistent spacing used throughout. Please normalize to using 2 spaces for indenting. Our internal protobuf style guide is located here.
| string string_value = 3; | ||
| int32 int_value = 4; | ||
| } | ||
| string label = 5; |
There was a problem hiding this comment.
Consider making the label and unit the first two fields with id 1 and 2 with all of the oneof field values following it. This way if you want to add more oneof field values in the future, the range of ids will still be consecutive. This perhaps borders a bit on the OCD side so I'm OK if we decide to leave as is. There are potential oddities either way if we decide to extend the messages in the future. However, if we are going to promote the types to this level of visibility, we should be pretty confident the types won't need to change in the future. Either that or we should add notes that clients should be mindful to handle value types that aren't currently defined by the message.
| option php_namespace = "NI\\PROTOBUF\\TYPES"; | ||
| option ruby_package = "NI::Protobuf::Types"; | ||
|
|
||
| message ParametricDataValue { |
There was a problem hiding this comment.
I'll admit the term parametric data doesn't quite land with me conceptually here. This essentially looks like named data with units to me. I recognize coming up with a good name for that is challenging and perhaps there isn't anything significantly better that warrants a change at this point. The best ideas I have at the moment are something like DataWithUnits or AnnotatedData with a file name of data.proto.
| string unit = 6; | ||
| } | ||
|
|
||
| message ParametricDataValueArray { |
There was a problem hiding this comment.
Have you considered collapsing this into the ParametricDataValue message value above? Is there some advantage to partitioning array data values separate from scalar data values? That type of info would be good to add as part of the message documentation.
| } | ||
|
|
||
| message ParametricDataValueArray { | ||
| message DoubleArray { |
There was a problem hiding this comment.
Something else to consider is whether we should extract these nested message definitions into array.proto and document them as convenience messages for use in oneof fields or declaration of 2D jagged arrays.
| message ParametricData { | ||
| ParametricDataValue value = 1; | ||
| repeated ParametricDataValue parameters = 2; | ||
| } |
There was a problem hiding this comment.
The meaning of this field isn't immediately obvious to me so I feel like documentation here would definitely be good. Is this meant to describes inputs/configuration that was used to produce the data value? If so, why are we limiting to scalar parameters producing a scalar data value and vice versa. Again, should we be looking to collapse scalar and array message types into a single message?
| double double_value = 1; | ||
| bool bool_value = 2; | ||
| string string_value = 3; | ||
| int32 int_value = 4; |
There was a problem hiding this comment.
I would recommend changing all of the field names around int32 values to also use `int32" in the name. This matches wrappers.proto and provides better extensibility/consistency if we want to support 64 bit integers in the future.
|
@nick-beer @lschroed Can one of you take a look at Jason's comments and either respond to them or create work items for updating the proto file to prepare it for this repo? I'm not yet familiar enough with the parametric data type to answer some of these. You don't need to update the file in this PR. If there's going to be a lot of work that comes out of this, I'll wait until everything is more finalized to add this file. |
What does this Pull Request accomplish?
Adds the
parametric_data.protofile used by the data store service.Why should this Pull Request be merged?
While this type has not been fully released yet, it is needed for a LabVIEW client that is in development. Generated grpc-labview code depends on this type, so making it accessible with the other types in the same location is desirable. It will almost certainly live in this repo once it is released and while it is not officially finalized, @nick-beer has said it is probably final and @dixonjoel has approved putting up this pull request to make it available prior to official release.
What testing has been done?
The data types are used in a C# server and client and work well.