RSDK-6829 - Add machine part id to ResourceName message #467
Conversation
cheukt
changed the title
| @@ -15,6 +15,7 @@ message ResourceName { | |||
| string type = 2; | |||
| string subtype = 3; | |||
| string name = 4; | |||
| optional string machine_part_id = 5; | |||
There was a problem hiding this comment.
everything else in the resource name should always be filled out, so using optional here to be a bit more clear about the optionality. It doesn't seem like we are super consistent about this though, so could see making it a non-optional field as well
There was a problem hiding this comment.
yeah, that was gonna be my next question - it doesn't seem like we are consistent with using the optional keyword vs relying on implicit optionality in protos (e.g. this field will just be a blank string)
There was a problem hiding this comment.
i don't have have a strong opinion on this, so won't block - I'm okay making this explicitly optional
There was a problem hiding this comment.
I'd like to start using optional a bit more, I think it makes it easier to tell what the response shape is like by just reading the proto
| @@ -15,6 +15,7 @@ message ResourceName { | |||
| string type = 2; | |||
| string subtype = 3; | |||
| string name = 4; | |||
| optional string machine_part_id = 5; | |||
There was a problem hiding this comment.
Do we not also want the machine ID? Or just the part ID?
There was a problem hiding this comment.
just part id for now, that's what was scoped and most likely to be useful - user can get cloud metadata if they need more of that stuff
…#467) Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
RDK change: viamrobotics/rdk#3757