spec: VolumeHandle replaces VolumeID, VolumeMetadata #97
Conversation
|
xref #71 |
spec.md
Outdated
| // example, the CO MAY generate log messages that include this data. | ||
| string id = 1; | ||
|
|
||
| // Metadata captures additional, possibly sensitive information about |
There was a problem hiding this comment.
nit: additional, possibly sensitive, information
There was a problem hiding this comment.
I am not sure i agree with this nit. To me it reads better without the extra comma.
There was a problem hiding this comment.
Having a single comma suggests that "Metadata captures additional" is an introductory clause. But, "possibly sensitive information about..." can't stand on it's on (it is incomplete without the first part of the sentence).
Instead, "possibly sensitive" is nonessential information. And the sentence reads fine without it.
From https://owl.english.purdue.edu/owl/owlprint/607/:
Commas with Nonessential Elements
Some modifying elements of a sentence are essential, restricting the meaning of a modified term, while others are nonessential and don't restrict the modified term's meaning. These nonessential elements, which can be words, phrases, or clauses, are set off with commas.
Rule: Use commas before and after nonessential words, phrases, and clauses, that is, elements embedded in the sentence that interrupt it without changing the essential meaning.
So I would offset "possibly sensitive" with two commas.
I'm being very pedantic here, so feel free to disregard =)
| // NOT change over the lifetime of the volume and MAY be safely cached | ||
| // by the CO. | ||
| // Since this object will be passed around by the CO, it is RECOMMENDED | ||
| // that each Plugin keeps the information herein as small as possible. |
There was a problem hiding this comment.
Maybe add in a size requirement, like we did with credentials. Always better to start with a hard limit and loosen it in the future, then not starting with a limit and trying to add it later. e.g. The total bytes of the ID and metadata must be less than 1 Mebibyte.
spec.md
Outdated
| // Since this object will be passed around by the CO, it is RECOMMENDED | ||
| // that each Plugin keeps the information herein as small as possible. | ||
| message VolumeHandle { | ||
|
|
There was a problem hiding this comment.
nit: kill this line to be consistent with other places.
|
LGTM. Thanks @jdef. Merging. |
Refactor volume ID and metadata; provide additional clarification.
xref #29 #88