Add comments to literal value message #2804
Conversation
| @@ -473,6 +473,7 @@ message LiteralValue { | |||
| ArrayLiteral array_value = 8; | |||
| MessageLiteral message_value = 9; | |||
| } | |||
| string description = 10; | |||
There was a problem hiding this comment.
What is this field? On face value, semantically a LiteralValue is very clear as to what it is. Unless I'm missing something, it seems unclear why it would have a description field.
There was a problem hiding this comment.
I suggested calling it description for consistency with everything else in this package (see lines 303 and 341 above for example).
But it comes from comments in the source for this option value. So when an option value has a description, it can be rendered in docs as a comment.
There was a problem hiding this comment.
description is intended to be any leading comments on the option/value. I can see why the name was confusing there. The idea was to reuse the same field name here for leading comments like we did for Message and the others. I can add some docs explaining what this is or we can rename the field to something comments to be more explicit. Wdyt?
There was a problem hiding this comment.
I don't understand the placement semantically. It seems we have descriptions on Enums, EnumValues, Services, Messages, Fields, but then not on Methods, and not on any of the other .*Literal.* types. Why is this Literal special (and why don't we have it on Method)?
Regardless, yes comments is a better name.
There was a problem hiding this comment.
(and why don't we have it on
Method)?
Maybe you overlooked it? It's there:
It's not on the other literal types since those are really just the concrete kinds of LiteralValue. If it were on those others, it would be redundant with the comment on the enclosing LiteralValue.
There was a problem hiding this comment.
Semantically, it's a comment about a particular option value. For example:
// A comment about the first element.
option (some_repeated_option) = { foo: "bar" };
// A comment about the second element.
option (some_repeated_option) = { foo: "baz" };
// A comment about a field value inside a message.
option (some_message_option).name = "Bob Loblaw";
// A comment about a simple scalar option value
option (some_scalar_option) = 42;There was a problem hiding this comment.
Got it - ok, let's just rename it to comments
There was a problem hiding this comment.
Another option we discussed was putting it on the FieldLiteral, so it's associated not just with a value but the value of a field/option/extension.
However, that doesn't work to capture comments on repeated options, like in the above example for the some_repeated_option custom option. If we only had it on the field (some_repeated_option), then we don't know what comment to use, since the source actually has two comments for the field, one for each element in the list value. So LiteralValue is the most usable/flexible placement.
There was a problem hiding this comment.
Renamed to comments
| @@ -473,6 +473,7 @@ message LiteralValue { | |||
| ArrayLiteral array_value = 8; | |||
| MessageLiteral message_value = 9; | |||
| } | |||
| string comments = 10; | |||
There was a problem hiding this comment.
You renamed this comments but not everything else - update everything to be consistent
There was a problem hiding this comment.
👍 got it, my bad I thought we only wanted to do it here. Fixed in latest commit.
There was a problem hiding this comment.
Uh, do we use JSON between the BSR web UI and the BSR server? If so, this is not a backwards compatible change and will break the current functionality for comments (older client code will be looking for a description property in the JSON payload).
I guess we don't have buf breaking enabled for this repo, since nothing complained about this?
There was a problem hiding this comment.
Only the UI uses this, and so assumedly @gilwong00 will update the BSR anyways. We don't check for breaking changes on alpha APIs.
There was a problem hiding this comment.
Yea, i have a PR to make these changes in the BSR so we shouldnt be affected for to long
gilwong00
changed the title
Add
descriptionfield toLiteralValuemessage