This repository has been archived by the owner on Sep 30, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sketch of SCIP-based GraphQL API #61217
Sketch of SCIP-based GraphQL API #61217
Changes from 5 commits
e50bb3b
e5b9e38
df4fb88
203e505
1949594
eebbd29
072bbc0
8e947b7
4032028
2146fed
570798a
9269229
7c42739
e90ab20
5f0c62a
dbd4068
e0af435
653a197
e20c0a5
38fc951
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Add documentation here describing how this can be used for the symbols side bar so that the symbols sidebar is consistent with the navigation in the file. E.g. you can have a precise symbols sidebar if you have precise code intel instead of having to use
type:symbol
search in the file.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Revert these changes, we don't care about this API anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Should we remove the
!
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Think through the non-null cases more carefully, especially for
UsageConnection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0
meaning "no value given" is a very go-ey thing, I think you don't need a default value here, sincesurroundingLines: SurroundingLines
already indicates nullable and the backend can then decide how to represent that in the given programming languageThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here,
0
doesn't mean "no value given", it literally means0
because that makes sense as a default -- only the single line from the blob will be returned in the default case. I wrote the 0 explicitly here as it serves as documentation for an API client for what the behavior will be if they omit this parameter. Another potential default would beINT32_MAX
(i.e. get the full blob contents), but we're deliberately picking0
here, notINT32_MAX
.