Organized completions #6351
Organized completions #6351
Conversation
matklad
left a comment
matklad
left a comment
There was a problem hiding this comment.
r=me with some pub replaced with pub(crate)
bors d+
|
|
||
| /// Represents an in-progress set of completions being built. | ||
| #[derive(Debug, Default)] | ||
| pub struct Completions { |
There was a problem hiding this comment.
Could some of these pubs be reduced to pub(crate)?
| mod config; | ||
| mod item; | ||
| mod context; | ||
| mod presentation; |
There was a problem hiding this comment.
I guess, I am -0 on this one. I do think it's a good idea to separate what items we are completing from how do we render each item into JSON completion, especially given that its very easy to mix the two. So, in this light, I would prefer presentation and specific completions be sibling modules, rather than presentation sutff being in the parent module.
But I also agree that, practically speaking, there isn't much difference here, so i am fine either way.
There was a problem hiding this comment.
I'm not sure if I understand what you mean, to be honest. It's likely that I miss some kind of a bigger picture, I considered this module as mostly definition of several methods of Completions that are used by actual completions modules, and can't get how it's related to the rendering -- it just manipulates CompletionItem objects, isn't it?
If by that you mean creating actual snippets, then unfortunately concrete completions provide snippets on their own as well.
What I don't like here is the score computation, but separating completions generation from the scoring is a topic for a different PR, IMHO.
There was a problem hiding this comment.
Yeah, I am describing two parts of big picture here.
One is that, when we are computing completions, we need to ask two mostly orthogonal questions.
- Can I complete function
foohere? (Ie, is this expression in the first place? isfooin scope? is it resolvable?) - What exactly does it mean to complete
foo? (Ie, should I add()? Should I place cursor between parenthesis or after them? What string do I display in completion widget?)
It is possible to code both of them simultaneously, by just calling Completions::add(CompletionItem) from corresponding completion handler. But I feel (but cannot strongly motivate it :-) ) that it's better to use Completions::add_function(hir::Function) in the handlers, and handle rendering of hir::Function to CompletionItem elsewhere. Maaybe we should even make Completions::add private, and add Completions::add_keyword and such instead for cases where we currently use low-level interface.
A kind of an even bigger picture is that the job of IDE layer is to take some semantic things (hir::Function, which is an integer id internally) and turn it into a POD of strings, vector and field-less enums (CompletionItem). That bit of POD is then converted to JSON and ultimately rendered for the user in the UI. I take a mental shortcut here, and just thing of hir -> pod transformation as rendering or presentation. This "rendering" is inherently lossy, so it's best to postpone it as far as possible and, for every feature, have a clear layer which is responsible for such rendering.
There was a problem hiding this comment.
I see, thanks! Yeah, now I agree with you, I guess I'll try to make this difference more clear soon, so that it will be distinguishable not by the context, but rather by types.
|
✌️ popzxc can now approve this pull request. To approve and merge a pull request, simply reply with |
|
bors r+ |
3 participants
This PR continues the work on refactoring of the
completionscrate.In this episode:
completionsmodule, so they aren't mixed with the rest of the code.complete_attribute=>completions::attribute,completion_context=>context).Completionsstructure was moved fromitemmodule to thecompletions.presentationmodule was removed, as it was basically a module withimplforCompletions.