Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Marks symbols in search-based usages as definitions#63509

Closed
kritzcreek wants to merge 0 commit into
mainfrom
christoph/search-based-symbol-kind
Closed

Marks symbols in search-based usages as definitions#63509
kritzcreek wants to merge 0 commit into
mainfrom
christoph/search-based-symbol-kind

Conversation

@kritzcreek

Copy link
Copy Markdown
Contributor

Closes https://linear.app/sourcegraph/issue/GRAPH-712/determine-usage-kind-for-search-based-usages-by-running-symbol-search

I've decided to run a separate symbol query for simplicity for now. I'll ask the search platform team if it would be more efficient to do it in a single query when I meet with them.

Test plan

Example query for the API console

@cla-bot cla-bot Bot added the cla-signed label Jun 27, 2024
@github-actions github-actions Bot added team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform labels Jun 27, 2024
@kritzcreek kritzcreek force-pushed the christoph/search-based-symbol-kind branch 2 times, most recently from 6105d29 to d392f39 Compare June 27, 2024 07:55
Comment thread internal/codeintel/codenav/service.go Outdated
Comment thread internal/codeintel/codenav/service.go Outdated
Comment thread internal/codeintel/codenav/service.go Outdated
Comment thread internal/codeintel/codenav/service.go Outdated
Comment thread internal/codeintel/codenav/service.go Outdated
Comment thread internal/codeintel/codenav/syntactic.go Outdated
Comment thread internal/codeintel/codenav/syntactic.go Outdated
Comment thread internal/codeintel/codenav/syntactic.go Outdated
Comment thread internal/codeintel/codenav/transport/graphql/root_resolver_usages.go Outdated
Comment on lines 82 to 81

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah this is because of "typed nil". https://go.dev/play/p/Xn8hLhjQnOb

Comment thread internal/codeintel/codenav/service.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's attach the counts for syntactic and search-based to the trace as an event (perhaps in the callers directly?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we're doing this once per file wouldn't this be a bit spammy? I'd maybe roll these up into a per-request event?

Comment thread internal/codeintel/codenav/service.go Outdated
Comment thread internal/codeintel/codenav/service.go Outdated
Comment thread internal/codeintel/codenav/service.go Outdated
Comment on lines 1288 to 1351

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's log the error instead of dropping it silently?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would lead to us double logging any error, as we've run syntactic search already before getting to this. Still in favor of logging here?

Comment thread internal/codeintel/codenav/service.go Outdated
Comment thread internal/codeintel/codenav/syntactic.go Outdated
@kritzcreek kritzcreek force-pushed the christoph/search-based-symbol-kind branch from d392f39 to 3a739ea Compare June 28, 2024 08:17
@kritzcreek

Copy link
Copy Markdown
Contributor Author

As this PR sits on top of #63464 I've fixed the feedback pertaining to that PR over there and rebased this one.

@kritzcreek kritzcreek changed the base branch from main to christoph/search-based-usages June 28, 2024 08:38
@kritzcreek kritzcreek force-pushed the christoph/search-based-symbol-kind branch from 3a739ea to 49aff24 Compare June 28, 2024 10:05
@kritzcreek kritzcreek force-pushed the christoph/search-based-usages branch from a9baaaa to 02ccd2d Compare June 28, 2024 10:10
@kritzcreek kritzcreek force-pushed the christoph/search-based-symbol-kind branch from 49aff24 to 62b1b0d Compare June 28, 2024 10:10
@kritzcreek kritzcreek force-pushed the christoph/search-based-usages branch from f3b044f to dd036d6 Compare June 28, 2024 10:33
@kritzcreek kritzcreek force-pushed the christoph/search-based-symbol-kind branch 2 times, most recently from a1abfe1 to e2af623 Compare June 28, 2024 13:46
Base automatically changed from christoph/search-based-usages to main June 28, 2024 13:46
@kritzcreek kritzcreek closed this Jul 1, 2024
@kritzcreek kritzcreek force-pushed the christoph/search-based-symbol-kind branch from 2003fd3 to e5d6a66 Compare July 1, 2024 09:36
@bobheadxi bobheadxi temporarily deployed to autobuildsherrif July 1, 2024 09:45 — with GitHub Actions Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants