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.

Sign up for GitHub

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

Add parametric data proto file #82

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add parametric data proto file #82

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions ni/protobuf/types/parametric_data.proto
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
//---------------------------------------------------------------------
//---------------------------------------------------------------------
syntax = "proto3";

//---------------------------------------------------------------------
//---------------------------------------------------------------------
package ni.protobuf.types;

//---------------------------------------------------------------------
//---------------------------------------------------------------------
option csharp_namespace = "NationalInstruments.Protobuf.Types";
option go_package = "types";
option java_multiple_files = true;
option java_outer_classname = "ParametricDataProto";
option java_package = "com.ni.protobuf.types";
option objc_class_prefix = "NIPT";
option php_namespace = "NI\\PROTOBUF\\TYPES";
option ruby_package = "NI::Protobuf::Types";

message ParametricDataValue {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

oneof value {
double double_value = 1;
bool bool_value = 2;
string string_value = 3;
int32 int_value = 4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}
string label = 5;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

string unit = 6;
}

message ParametricDataValueArray {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 DoubleArray {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

repeated double values = 1;
}
message BoolArray {
repeated bool values = 1;
}
message StringArray {
repeated string values = 1;
}
message IntArray {
repeated int32 values = 1;
}
oneof value {
DoubleArray double_array = 1;
BoolArray bool_array = 2;
StringArray string_array = 3;
IntArray int_array = 4;
}
string label = 5;
string unit = 6;
}

message ParametricData {
ParametricDataValue value = 1;
repeated ParametricDataValue parameters = 2;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?


message ParametricDataArray {
ParametricDataValueArray values = 1;
repeated ParametricDataValueArray parameters = 2;
}