-
Notifications
You must be signed in to change notification settings - Fork 1.3k
allow contributing actions to location results #1627
Conversation
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.
I'm a bit bothered by this in its current implementation. For users having basic-code-intel + other language extensions installed, this will fail to reliably differentiate fuzzy refs from lsp refs (because the badge is shown at the file header level, and not at the reference level).
Worse, it might lead users to think that all they're getting is fuzzy references, and that their other language are not working correctly, since they might end up with fuzzy badges on all file matches, even though there are some LSP matches in there.
With this in mind, rethinking how matches are displayed to show contributions at match-level and not at file header level is needed and not a second step / nice to have IMO. A nice-to-have would be to also show a badge on the file header if all locations in a file match have the same contribution (ie. all matches are "fuzzy")
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.
With this in mind, rethinking how matches are displayed to show contributions at match-level and not at file header level is needed and not a second step / nice to have IMO.
Yeah, on second thought, you are right. Precise matches are almost always going to be a subset of fuzzy matches.
@francisschmaltz Any ideas here? What about showing references in the same way VS Code does: showing 1 line per match, and making the "snippet area" a full editor that scrolls to the line of the match? This would add a new column (or a new level of hierarchy to an existing column) in the panel.
I will think of ideas...we need to figure something out by 3.0.
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.
@lguychard Besides this UI stuff, what do you think of the rest of the PR (the extension API changes)? My assumption for all the UI solutions is that they would use these same API changes, so most of this PR will remain the same.
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.
The rest LGTM
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.
Unfortunately, I don't think this will be possible to finish in time to get merged. It requires significant changes to the way results are shown in the panel.
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.
Now that basic code intel has been split into N language extensions, showing the badge at the file header level is OK. You'll never see mixed fuzzy + precise results.
@sqs From a product perspective, are there any other items blocking this?
From a technical perspective, I bet this needs to be migrated to the post-Comlink world and some merge conflicts fixed up. I'll optimistically start on this until someone chimes in.
3907f04 to
104cb81
Compare
This lets extensions' location providers (definition/reference/etc. providers) associate context data with each location, and also contribute actions that are shown selectively in the file title for location matches shown in the panel. Together, these new features allow the basic-code-intel extension to add "Fuzzy" badges to defs/refs in the panel. This fixes sourcegraph/sourcegraph#1174. It would also enable things like: - Showing a "source" badge on cross-repository reference results explaining how the reference was found (eg by looking up dependents on npm) - Showing the type of reference (eg denoting some references as "Assignments", some as "Calls", some as "References", etc.) - Adding an action to "hide" or "always ignore" the reference - Adding an action to add the match to a saved list - Adding an action for users to say "This was useful" or "This was not useful" (eg for a code examples extension) See sourcegraph/code-intel-extensions#10 for an example of how extensions can use this new API.
…not in FileMatch header This turned out to be more complicated than I thought.
|
You are right that the new way of doing basic code intel makes this simpler. I think we can actually do this more simply now, without this PR, because there is much less mixing of fuzzy and non-fuzzy.
You can make the basic code intel mode add an action item to the panel header to indicate “Imprecise results” with a description like “These locations are computed using heuristics. (If available:) use a language server for precise results.” The action would be a link to the docs for that language on setting up a language server if available, and otherwise a link to the docs about the limitations of that language’s basic code intel mode.
This would require only a code change to the extensions and not the extension API.
Wdyt?
|
|
Yeah, that's even simpler, and makes it clear that ALL of the defs/refs in the panel are precise or imprecise. Would it be implemented using https://github.com/sourcegraph/sourcegraph/blob/master/doc/extensions/authoring/builtin_commands.md#executelocationprovider ? I'll give that a shot. |
|
It would just be an action contribution to the panel, not execute location provider. |
|
Yep |
|
Closing this now that I implemented "Imprecise results" action item in the panel in sourcegraph/code-intel-extensions#41 |


This lets extensions' location providers (definition/reference/etc. providers) associate context data with each location, and also contribute actions that are shown selectively in the file title for location matches shown in the panel.
Together, these new features allow the basic-code-intel extension to add "Fuzzy" badges to defs/refs in the panel. This fixes https://github.com/sourcegraph/sourcegraph/issues/1174.
It would also enable things like:
See sourcegraph/code-intel-extensions#10 for an example of how extensions can use this new API.
Screenshot (note the Fuzzy badge in the panel)