-
Notifications
You must be signed in to change notification settings - Fork 14
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
Refactor completions #754
Refactor completions #754
Conversation
5db87e7
to
fe58072
Compare
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.
Nice progress! I quite like the way sources are now structs that implement a common trait. However it feels like we could improve the way things are structured by:
- Removing the
build()
method from the builder, and renaming the builder to something more like "state" or "context". - Not having the aggregators implement the source trait
- Using a generic function to statically dispatch on sources instead of for loops with dynamic dispatch.
See comments for justifications on these thoughts. Happy to discuss these points during our casual!
let root = find_pipe_root(self.context)?; | ||
|
||
// Cache it for future calls (ignore failure if race condition, which shouldn't happen) | ||
let _ = self.pipe_root_cell.set(root.clone()); |
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.
We should at least panic (e.g. via expect()
) or log an error if the assumption isn't satisfied.
But we could just use get_or_init()
to avoid this I think? This would be the more idiomatic use of once cell.
Also I think the cell should contain the result so that we don't try to recompute the root when there is an error?
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 mostly implemented this and certainly think I've gotten at the spirit of it. If you have concerns with how I handled it, we can refine.
Allows us to iterate in a clean way over unique sources
Finally all completion sources are shifted over to a new, common pattern. Some return types were changed (to Option<Vec<CompletionItem>>) as a move towards consistency in completion source functions
Remove on-entry logging for completion sources, since the 2 main aggregators now log before trying each source Shift towards debug for logging from a completion source or related helpers
* CompletionBuilder -> CompletionContext * context -> document_context (where necessary for disambiguation)
0ebe2b9
to
6ef4fef
Compare
dd70c8d
to
ecea7ad
Compare
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.
Looks great!
Relates to #681
The main goal is to introduce a new struct to serve as headquarters when generating completions. This gives us a home for features that we compute about the completion site, that can then be used downstream in one or multiple concrete completion sources. Two existing examples of this, each previously implemented in different ways, are:
Qualities we wanted in this refactoring:
I'll make a few comments on the source as well.
(I've drafted a 2nd PR #755 that builds on this one, that proposes changes to how we manage completion items as they roll in from various sources. That's in draft form until the dust settles here.)