Introduce dedicated SignatureDocumentation message type#385
Introduce dedicated SignatureDocumentation message type#385
SignatureDocumentation message type#385Conversation
Replace the reuse of Document for SymbolInformation.signature_documentation with a dedicated SignatureDocumentation message. The new message contains only the relevant fields (language, text, occurrences) and reserves Document's field numbers (1, 3, 6) to maintain wire compatibility with older indexes.
jupblb
force-pushed
the
michal/signature-doc
branch
from
d3d2e5c to
9073268
Compare
CatherineGasnier
left a comment
CatherineGasnier
left a comment
There was a problem hiding this comment.
Welcome change!
| // 4. The path must use '/' as the separator, including on Windows. | ||
| // 5. The path must be canonical; it cannot include empty components ('//'), | ||
| // or '.' or '..'. | ||
| string relative_path = 1; |
There was a problem hiding this comment.
I wonder if some indexers were abusing this field to link a symbol to its containing file? In general, do we usually do bidirectional linking between messages? Right now a Document has a symbols field, but SymbolInformation does not point to its containing Document. Should it? Not super concerned by this but just raising.
There was a problem hiding this comment.
I'm against having bidirectional linking and I think this convention fits the rest of the protocol definition. SCIP format is intended to be an intermediate form that is to be processed further. I know about only a single indexer that sets this field - but we mark it as reserved so it should not be a problem.
scip.proto
Outdated
| // encoded signatures using the Document message type. | ||
| message SignatureDocumentation { | ||
| // The language of the signature, e.g. "java", "go", "python". | ||
| string language = 4; |
There was a problem hiding this comment.
I'm wondering if this should be a field for Document only, to avoid the possibility of creating inconsistent data. But just nitpicking.
There was a problem hiding this comment.
There were two reasons I've left it here:
- When we render hovers with signature in the Sourcegraph UI, we support syntax highlighting. It's easy to just get information about the language from this field. But it's not a problem at all to source it from the
Documentcontext. - Within boundary of a single language this is not a useful field, which is always the case right now. But if we were to support cross-language references then it would make more sense, maybe.
I am fully ok with removing this field if you think there is no point in keeping it.
2 participants
SymbolInformation.signature_documentationpreviously reused theDocumentmessage type, even thoughDocumentis designed to represent a source file on disk (with a "required"relative_path, etc.). This PR introduces a dedicatedSignatureDocumentationmessage with a subset ofDocumentfields.The new message preserves the same protobuf field numbers as
Documentfor the shared fields, so old and new indexes are binary wire-compatible in both directions. TheDocument-only field numbers arereservedinSignatureDocumentationto prevent accidental reuse.This change is not compliant with Protobuf best practices (shouldn't change the type of an existing field) but we're also pre-1.0.
Supersedes #299