[symbols-view] Allow project-wide symbol searches to consider multiple providers #1133
Conversation
…multiple providers instead of enforcing exclusivity.
|
Once again, the one failing editor test is the known-flaky one, so I won't make a machine in a data center do any unnecessary work. This is the corresponding fix on the |
There was a problem hiding this comment.
I wish I were more deeply familiar with providers and symbols, of course.
But at a quick look, the updated JavaScript logic in packages/symbols-view/lib/provider-broker.js looks sound and like it would activate under the intended scenario based on whether the param {enforceExclusivity: true/false} has been passed, and furthermore will safely default to true as intended if said param is unset when calling select().
I tested equivalent syntax in Node REPL to confirm.
And this PR has added specs, which is always appreciated. (Can confirm this PR's one test failure is indeed the known issue affecting multiple unrelated PR's, so not this PR's fault.)
I leave the desirability of the change to those who know more about symbols and providers of same, but I note a 👍 reaction from a community member on the corresponding issue, who has been fairly engaged in Pulsar things, so I can only hope that means this is moving in the correct direction.
I'm comfortable enough with this moving forward in that the code looks likely to behave as intended and seemingly has support from at least two people and no dissents yet. At this rate, good enough!
Having explained myself possibly at too much length, here is my approving review. 👍
|
Merging now! Thanks. |
Fixes #1117.
Identify the Bug
Described in #1117. You should be able to get project-wide symbol search results no matter which sort of file is open in your active editor. For instance, you should be able to get JavaScript results even if you're currently editing a
package.jsonfile.Description of the Change
We usually pick a single winner among providers (unless they mark themselves as “non-exclusive” so that they can supplement the result set) in an effort to reduce duplication of effort. When we ask providers for project-wide results, though, we won't enforce exclusivity; there are lots of polyglot projects and we're interested in all possible results.
This removes the artificial narrowing of the result set on our end. But half of this needs to be done in the providers. For instance,
pulsar-ide-typescriptshould sometimes consider metadata about the active editor, but sometimes shouldn't. If the metadata indicates that we're trying to resolve a “go to declaration” command, then it should decline to act as a provider unless the user is in a TS or JS file; but if we're doing project-wide symbol search, then the active editor is far less relevant, and it should be willing to contribute TS/JS symbols to the full result set along with other providers.I'll be making this change in
@savetheclocktower/atom-languageclientso that IDE provider packages do the right thing out of the box.Alternate Designs
This is the only solution that works here; even some other design would've needed us to explicitly remove this constraint on the symbol result set.
Possible Drawbacks
We're lucky that neither of the built-in providers is able to resolve project-wide symbol searches, or else this change could potentially introduce lots more noise to the result set. It's rare, but possible, that the user would have two different IDE packages installed that are able to supply symbol information for the same language — and if both are active, then both will end up in the result set.
The workaround for the user would theoretically be to configure one of those packages not to try to provide symbols; this may or may not be present in that package's settings, but we could handle these situations when they occur in real life instead of just existing as theoreticals.
Verification Process
Specs should pass.
Release Notes