This repository has been archived by the owner on Sep 30, 2024. It is now read-only.
client: Centralize ref panel logic for Location grouping & uniquing #58183
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
varungandhi-src
force-pushed
the
vg/refpanel-centralize
branch
from
5b9e6cb to
1a15900
Compare
varungandhi-src
commented
Nov 9, 2023
varungandhi-src
commented
Nov 9, 2023
varungandhi-src
commented
Nov 9, 2023
| }, | ||
| references: { | ||
| endCursor: lsif.references.pageInfo.endCursor, | ||
| nodes: dedupeLocations(lsif.references.nodes).map(buildPreciseLocation), | ||
| nodes: new LocationsGroup(lsif.references.nodes.map(buildPreciseLocation)), |
There was a problem hiding this comment.
Technically, this is a bit suboptimal because we're mapping before de-duping, but it should be fine in most cases as we generally have < 50 references, and we have a pagination limit which prevents this from growing quickly. Thoughts on if I should change it so that the mapping happens after filtering regardless?
There was a problem hiding this comment.
I don't think it will have any measurable impact.
- Fixed some local variable names - Added more assertions - Added more comments
mrnugget
reviewed
Nov 10, 2023
Otherwise, the call-sites read weirdly due to property syntax a.getX.b
varungandhi-src
force-pushed
the
vg/refpanel-centralize
branch
from
53c4290 to
5325035
Compare
fkling
approved these changes
Nov 14, 2023
| }, | ||
| references: { | ||
| endCursor: lsif.references.pageInfo.endCursor, | ||
| nodes: dedupeLocations(lsif.references.nodes).map(buildPreciseLocation), | ||
| nodes: new LocationsGroup(lsif.references.nodes.map(buildPreciseLocation)), |
There was a problem hiding this comment.
I don't think it will have any measurable impact.
varungandhi-src
force-pushed
the
vg/refpanel-centralize
branch
from
d10743b to
a0c03cf
Compare
vovakulikov
pushed a commit
that referenced
this pull request
Dec 12, 2023
…58183) This way, we can document invariants and add assertions in a central places.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The current ref panel code scatters calls for grouping and uniquing in a bunch of different places.
This makes the code hard to understand (which invariants hold and why?) and makes it harder
to propagate information easily at per-file or per-repo granularity.
In the future, we want to attach one SCIP Document per file for https://github.com/sourcegraph/sourcegraph/issues/58182
So I've attempted to centralize the grouping and uniquing logic here, and made
liberal use of assertions and comments to make it clear which invariants hold.
My coding style + naming is probably somewhat unidiomatic, open to suggestions for improvements.
Test plan
TODO, not sure what kinds of tests I should add. Unit tests for the new
LocationGroupclass?Is there any easy way to add fuzz tests to make sure that no assertions are hit?