Search-based usages for new codenav API#63464
Conversation
3c67ec0 to
884e3d8
Compare
d27bd6e to
70a9171
Compare
05c4691 to
f3b044f
Compare
Also makes the usage symbol optional
it doesn't matter if we ran syntactic search in this request or not, we don't want to show syntactic matches as search-based
f3b044f to
dd036d6
Compare
|
@varungandhi-src I think I addressed all the feedback you gave in the other PR over here. Could you take another look? |
varungandhi-src
left a comment
There was a problem hiding this comment.
Please simplify some of the code before merging.
| var syntacticUploadID int | ||
| if previousSyntacticSearch.Found { | ||
| syntacticUploadID = previousSyntacticSearch.UploadID | ||
| } else { | ||
| syntacticUpload, err := s.getSyntacticUpload(ctx, trace, repo, commit, path) | ||
| if err != nil { | ||
| trace.Info("no syntactic upload found, return all search-based results", log.Error(err)) | ||
| } else { | ||
| syntacticUploadID = syntacticUpload.ID | ||
| } | ||
| } |
There was a problem hiding this comment.
AFAICT, there is no data dependency between this if and the findCandidateOccurrencesViaSearch call. Please combine all the if previousSyntacticSearch.Found checks in one place, so it's clear at a glance. If you rename the type to something more generic, you can fold the logic into a single immediately-invoked-func that returns struct { uploadID, symbolName, language }, error, and only that func needs to look at previousSyntacticSearch
|
|
||
| results := [][]SearchBasedMatch{} | ||
| for pair := candidateMatches.Oldest(); pair != nil; pair = pair.Next() { | ||
| if syntacticUploadID != 0 { |
There was a problem hiding this comment.
I think 0 is potentially a valid UploadID? Please don't check for 0 here.
| if identifier == "" { | ||
| return *orderedmap.New[core.RepoRelPath, candidateFile](), nil | ||
| } |
There was a problem hiding this comment.
If we're getting an empty string here, that's likely a mistake in the API client, right? In that case, we should return an error from this function, or the caller should guarantee that the identifier is non-empty.
| SnapshotForDocument(ctx context.Context, repositoryID int, commit string, path core.RepoRelPath, uploadID int) (data []shared.SnapshotData, err error) | ||
| SCIPDocument(ctx context.Context, uploadID int, path core.RepoRelPath) (*scip.Document, error) | ||
| SyntacticUsages(ctx context.Context, repo types.Repo, commit api.CommitID, path core.RepoRelPath, symbolRange scip.Range) ([]codenav.SyntacticMatch, *codenav.SyntacticUsagesError) | ||
| SyntacticUsages(ctx context.Context, repo types.Repo, commit api.CommitID, path core.RepoRelPath, symbolRange scip.Range) (codenav.SyntacticUsagesResult, codenav.PreviousSyntacticSearch, *codenav.SyntacticUsagesError) |
There was a problem hiding this comment.
Perhaps we can bundle the repo, commit, path, symbolRange into a ResolvedRangeInput analogous to the distinction between UsagesForSymbolArgs and UsagesForSymbolResolvedArgs?
|
I think I got them all, but if I missed a comment I'm happy to implement it over in #63509 next week |
Closes https://linear.app/sourcegraph/issue/GRAPH-703/syntactic-filtered-search-based-usages
This initial implementation adds search-based usages for the new codenav API.
I'll work on a follow-up PR(https://github.com/sourcegraph/sourcegraph/pull/63509) to get us to feature parity with the current search-based implementation. https://linear.app/sourcegraph/issue/GRAPH-712/determine-usage-kind-for-search-based-usages-by-running-symbol-search
Additionally I've added an implementation note for when I work on pagination next cycle here: https://linear.app/sourcegraph/issue/GRAPH-696/implement-pagination-for-syntactic-and-search-based-usages
Test plan
Using the API console