-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Move completions rendering into a separate module #6430
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
|
Looks good to me, this is definitely a direction worth exploring. I like the stricter separation, and I like that each thing gets its dedicated module. I slightly don't like the proliferation of types here. There's a natural tension between "every bit of logic is separated into a module" and "there's only few vocabulary types". In an ideal world, there should be many pieces, but they all should operate the same types. I am also not sure if super-strict separation would be benefitial long-term (although i expect that it would). The only not-so-minor complaint I have is about do-er objects, the rest looks good to me! bors d+ |
|
✌️ popzxc can now approve this pull request. To approve and merge a pull request, simply reply with |
62edb10 to
8efe432
Compare
|
bors r+ |
This PR extracts rendering-related things from
Completionsstructure to the newrendermodule.rendermodule declares aRenderstructure (which is a generic renderer interface),RenderContext(interface for data/methods not required for completions generating, but required for rendering), and a bunch of smaller*Renderstructures which encapsulate logic behind rendering a certain item.This is just a step in full separation direction, since the following this are still to be done:
CompletionContextto theRenderContext;rendermodule;This PR is already pretty big, so not to make it even harder to review I decided to split this process into several subsequent PRs.