Resolve ignore comments for group fields - #4684
Conversation
| case parentMessageDescriptor != nil && tag == messageNestedMessagesTag: | ||
| messageDescriptors = parentMessageDescriptor.Messages() | ||
| default: | ||
| return nil |
There was a problem hiding this comment.
While I think source paths are supposed to be valid here, I kept things lenient here since it seemed better that way
There was a problem hiding this comment.
This seems like a reasonable approach.
emcfarlane
left a comment
emcfarlane
left a comment
There was a problem hiding this comment.
lgtm! Small suggestions but feel free to resolve.
| case parentMessageDescriptor != nil && tag == messageNestedMessagesTag: | ||
| messageDescriptors = parentMessageDescriptor.Messages() | ||
| default: | ||
| return nil |
There was a problem hiding this comment.
This seems like a reasonable approach.
| // A source path for a field declaration alternates a tag and an index, descending through | ||
| // message declarations before terminating at a field or an extension field, for example | ||
| // [4, 0, 3, 1, 2, 0] for .message_type(0).nested_type(1).field(0). | ||
| func fieldDescriptorForSourcePath( |
There was a problem hiding this comment.
Would it be helpful to export a utility from protosourcepath for source path parsing/traversal, since those mechanisms already exist? I think that might help simplify the logic required for maintenance at this layer.
There was a problem hiding this comment.
Thanks - I have extracted a helper, in doing so made it a bit more general (previously we only traversed for a group field, not any type of descriptor
Co-authored-by: Edward McFarlane <3036610+emcfarlane@users.noreply.github.com>
3 participants
Currently ignore comments are missed when a group is defined inline on a field. This PR takes the approach exactly outlined, to resolve a source path in the descriptor, check if it's a group, and check it's message type for comments instead of only the field. Not ideal, but it's only needed on errors and should still generally be a relatively short traversal.
Fixes #4187