Skip to content
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

Refactored LanguagesMainImpl and OutputChannelRegistryMainImpl so they could be extended #6148

Merged
merged 1 commit into from
Sep 18, 2019

Conversation

JPinkney
Copy link
Contributor

@JPinkney JPinkney commented Sep 9, 2019

What it does

This PR introduces LanguagesMainFactory and an OutputChannelRegistryFactory so that we can extend LanguagesMainImpl and OutputChannelRegistryMainImpl in order to provide metrics for the plugin extension.

See: this for the implementation way and this for the whole discussion.

How to test

This PR doesn't include any new functionality, more just a restructuring of everything that enables it to be extended more.

With that said, you can test by installing a vscode extension that has lots of features such as vscode-java and testing whether or not the language features work on a simple project such as: https://github.com/che-samples/console-java-simple.

For reference, I tested auto completion, validation, find all references, go to definition, go to symbol, go to type definition, format document, refactor, rename symbol and everything was working as expected.

Additionally, if you want to see how it's extended you can build and use this theia extension: https://github.com/JPinkney/theia-plugin-extension-metrics

Review checklist

Reminder for reviewers

@JPinkney JPinkney requested a review from a team as a code owner September 9, 2019 19:29
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should put LanguagesMainImpl and OutputChannelRegistryMainImpl in DI context, i.e. they should not receive a container anymore, but be injectable and inject required services.

@akosyakov akosyakov added the plug-in system issues related to the plug-in system label Sep 10, 2019
Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the largest change is unrelated to what we're trying to achieve. I would revert that change and focus on the essential.

@@ -105,23 +108,39 @@ export class LanguagesMainImpl implements LanguagesMain {
position: monaco.Position,
context: monaco.modes.SuggestContext,
token: monaco.CancellationToken): Thenable<monaco.modes.ISuggestResult> =>
Promise.resolve(this.proxy.$provideCompletionItems(handle, model.uri, position, context, token)).then(result => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm not against this refactoring, it's completely unrelated to what you're trying to achieve in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's related, one can subclass LanguagesMainImpl and override provideCompletionItemsImpl to wrap it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the idea was to refactor and then subclass that way I can easily wrap the requests while making minimal changes to the code.

That way I can do something like https://github.com/JPinkney/theia-plugin-extension-metrics/blob/master/src/browser/plugin-metrics-languages-main.ts#L40 and have that piece of code be called by https://github.com/eclipse-theia/theia/pull/6148/files#diff-42e21ff47aae4b2d39ad311d392f4a26R109

@akosyakov
Copy link
Member

@JPinkney please rebase again

@JPinkney JPinkney force-pushed the languagesMainRefactor branch 3 times, most recently from ca0e562 to 64142e1 Compare September 13, 2019 18:57
@JPinkney
Copy link
Contributor Author

@akosyakov Done!

Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactoring looks good to me, also verified that VS Code extensions are still working with Go extension.

@akosyakov
Copy link
Member

@JPinkney @tsmaeder are you going to merge it? Also adding @theia/plugin-metrics extension to this repo is fine.

@tsmaeder
Copy link
Contributor

@JPinkney you should have the commit rights now, correct?

@JPinkney
Copy link
Contributor Author

@tsmaeder I think I'm supposed to but after signing everything with eclipse I still wasn't granted any extra permissions. I'm going to try to get in contact with someone there to see what I can do

@JPinkney
Copy link
Contributor Author

Emailed the eclipse webmaster so now I have rights for everything. I'll merge if theres no objections

@JPinkney JPinkney merged commit 286944f into eclipse-theia:master Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants