protoparse: enforce unique JSON names #523
Conversation
|
@amckinney, mind taking a look at this? |
| if name != existing.orig { | ||
| otherName = fmt.Sprintf(" %q", existing.orig) | ||
| } | ||
| conflictErr := errorWithPos(fldNode.Start(), "%s: %s JSON name %q conflicts with %s JSON name%s of field %s, defined at %v", |
There was a problem hiding this comment.
| conflictErr := errorWithPos(fldNode.Start(), "%s: %s JSON name %q conflicts with %s JSON name%s of field %s, defined at %v", | |
| conflictErr := errorWithPos(fldNode.Start(), "%s: %s JSON name %q conflicts with %s JSON name %s of field %s, defined at %v", |
There was a problem hiding this comment.
I would have thought we'd need a space here, but is there always a space in the value we're including here? By the error messages included in your test, everything looks correct, so I'm a little surprised.
This would be the existing.source.GetName() value.
There was a problem hiding this comment.
We only print a name here when the two names that conflict differ (which can happen if they vary only by case). So if the previously defined JSON name is the same as the one causing the conflict, we omit it entirely. See how the name var is actually constructed above: if it's not empty, it includes a leading space.
There was a problem hiding this comment.
Ah ha! Thanks for the clarification - makes sense.
There was a problem hiding this comment.
FWIW, this is actually for the origValue value. The existing.source.GetName() is the next %s in the format string.
Port jhump/protoreflect#523 from protoparse over to protocompile.
Port jhump/protoreflect#523 and jhump/protoreflect#523 from protoparse over to protocompile. Co-authored-by: Joshua Humphries <jhumphries@buf.build>
Fixes #358
This is done in a case-insensitive way, just as
protocdoes it. However, I wish I knew why the protobuf team chose to make it case-insensitive. Case-sensitivity issues should generally be warnings or linter errors, not compile errors. 馃し