-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add support for excluding imports from symbol search #19996
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Should I add a test somewhere? |
Yes, please add a test. The place to add the test will be crates/ide-db/src/symbol_index.rs. Call |
I added a test using |
What do you mean by "local roots"? |
Ah, I see now. Probably because it wasn't needed. You can leave it as-is or set |
|
Can you squash? 5 commits is a lot. |
yes ofc, do you want one commit for everything? |
Yes. |
done |
Thanks! |
Post-review comment. This seems a bit under-documented and ambiguous as to what it does. I think it handles not selecting the "wrong" imports from re-exported values, but I have no idea what that criteria is (even having read the associated issue). Consider a follow up PR that adds docs that describe the problem and what the setting does to address it, include examples etc. Consider adding some of this to the first PR comment here too. (I got here via the changelog). |
Maybe a small table helps clarify
Note that the above rules were not modified by this feature PR at all. This PR adds a new config option rust-analyzer.workspace.symbol.search.excludeImports. Personally I think the default behaviour might be a bit confusing, but the feature in this PR is really simple and doesn't need much documentation. |
Got it, thanks for the details here. My confusion was over an assumption that only re-exports were to be excluded from the linked issue, and hence the naming of the option, and the description not mentioning re-exports seemed weird. That this PR excluded renames as well was the missing part of the puzzle that made it make sense. I'd encourage adding that wording to the description, as it will make the option easier to search for in VSCode settings (e.g. someone searching for re-export, or rename), and clarifies the behavior better than the short description currently there. (note docs.rs uses the terminology "re-export", I'd assume that term is probably more familiar to many than "public import"). |
This implements the feature proposed in #19976