-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Trait Import completion for Notable Traits #16245
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
5e409d6
to
022ee7d
Compare
Yes that looks like roughly what i had in mind |
☔ The latest upstream changes (presumably #16258) made this pull request unmergeable. Please resolve the merge conflicts. |
022ee7d
to
1052813
Compare
Well that was hard to fix the conflicts. Thanks for that now Anyways I had some questions, we are currently storing |
Gonna investigate whether we need to do this actually or not. We might not after all (but we can still make use of the notable trait stuff in another way, for completion priority) |
☔ The latest upstream changes (presumably #16267) made this pull request unmergeable. Please resolve the merge conflicts. |
Ye okay after reconsidering we don't need to do this hack, we can just remove the limit in play here. What we can do though is prioritize method ocmpletions that come from notable traits by adding an extra score field to https://github.com/veykril/rust-analyzer/blob/4f75e0f9108192a8d3e115e885cfaf220d1660b8/crates/ide-completion/src/item.rs#L131 Are you interested in doing that instead? |
Oh yes sure I can do that. Just learning more sections in RA :). So should I close this PR and recreate a new one?! |
Yes lets close this, just ask any questions on zulip, I think you've been there before. |
cc @Veykril
ImportMap
andImportInfo
are changed to record information related tonotable_traits
. Does the current implementation look okay? If so I can proceed with actual implementation of the completion.P.S: sorry if I shouldn't ping