You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
grpc-device package names and directory structure don't conform to most standard guidelines and those being adopted by NI internally. See Uber Protobuf Style Guide for additional information. While we don't intend to adopt everything from that style guide, the information pertaining to versioning and directory structure is still relevant. Specifically, issues we should consider addressing are:
The package names don't include version information (e.g. end in v1). This isn't the worst thing since we can always introduce v2 later if we need to break compatibility, but it's still out of the norm.
The package names don't really conform to standard hierarchical naming. In particular, proto files originating from NI should start with "ni.".
The imports of proto files from grpc-device assume a flat directory structure that doesn't include the package name. This has trickle down effects to other proto files that need to import proto files from grpc-device, and want to do so in a standard way. In particular session.proto is used in other services and exposed as part of their API. For example, measurementlink uses session.proto here and here.
I think there are a few options for how we can improve things here:
Update grpc-device to generate proto files in folders that match current package names and update all import statements for grpc-device to use the full path to the import which includes the package name. This may break existing client builds of our proto files, but it doesn't technically break source compatibility or wire compatibility. This may also have some ramifications when generating Python stubs since it apparently checks the content for a given proto file is identical across all usages. This is probably the least invasive options that at least conforms to standards for the package names we already have.
In addition to the previous option, update/fix all of the current package names so that they conform to standards/guidelines we want to adopt going forward. For example, I would envision package names that look something like:
ni.driver.daqmx.v1
ni.driver.dmm.v1
ni.driver.dcpower.v1
ni.driver.session.v1
Some combination of 1 and 2 above, but also continue to generate proto files in their current form or possibly move a snapshot of them into the current source and no longer add new features to them. The server implementation would then support endpoint mappings for both proto files for compatibility purposes.
In my mind, option 2 is ideal, and it really depends on how much we value backward compatibility at this relatively early stage. Packaging for session.proto is probably the least clear as there are a number of ways you could package it that seem reasonable.
Include it in its own stand alone versioned package: ni.driver.session.v1
Include it as part of a standard types package. This could be done either in a versioned or non-versioned package. See Google's AIP-213 and AIP-215 for their guidelines. Using this as a guide, the package name might be "ni.driver.type" where the message is named "Session". This same package could also house the messages currently defined in nidevice.proto.
grpc-device package names and directory structure don't conform to most standard guidelines and those being adopted by NI internally. See Uber Protobuf Style Guide for additional information. While we don't intend to adopt everything from that style guide, the information pertaining to versioning and directory structure is still relevant. Specifically, issues we should consider addressing are:
I think there are a few options for how we can improve things here:
In my mind, option 2 is ideal, and it really depends on how much we value backward compatibility at this relatively early stage. Packaging for session.proto is probably the least clear as there are a number of ways you could package it that seem reasonable.
AB#2279579
The text was updated successfully, but these errors were encountered: