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

Pass pluginInfo through to output channel append method #6312

Merged

Conversation

JPinkney
Copy link
Contributor

@JPinkney JPinkney commented Oct 2, 2019

Signed-off-by: Josh Pinkney [email protected]

What it does

This PR passes the pluginInfo through to the output-channel-registry-main $append method.

This is needed for https://github.com/eclipse-theia/theia/pull/6303/files#r330382601 to make the language plugins we can get metrics from more dynamic.

How to test

Put a breakpoint https://github.com/eclipse-theia/theia/compare/master...JPinkney:pass-plugin-info-output-registry?expand=1#diff-11ebe3cff728b3126a471050a7378864L37 and launch the browser backend/frontend. Then install vscode java. When it's installed and starts outputting to the output channel the breakpoint will be hit. Inspect the pluginInfo variable and you will find the pluginInfo that was passed through.

Review checklist

Reminder for reviewers

@JPinkney JPinkney added the plug-in system issues related to the plug-in system label Oct 2, 2019
@@ -991,7 +991,7 @@ export interface PreferenceRegistryExt {
}

export interface OutputChannelRegistryMain {
$append(channelName: string, value: string): PromiseLike<void>;
$append(channelName: string, value: string, pluginInfo: PluginInfo): PromiseLike<void>;
Copy link
Member

Choose a reason for hiding this comment

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

In general, I have two comments:

  1. should we make pluginInfo optional?
  2. should it simply be in the form of options? any so we can potentially pass more in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically we could make it optional, but I was just following what I did in: f25652a

Copy link
Member

Choose a reason for hiding this comment

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

Okay fair enough :)

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.

code-wise looks good to me, output channels still work

Only worry about additional information to send, but it can be optimized when we have a real issue.

@akosyakov akosyakov requested review from evidolob and benoitf October 3, 2019 04:30
@JPinkney
Copy link
Contributor Author

JPinkney commented Oct 9, 2019

@akosyakov Is it okay to merge or should we wait for @evidolob and @benoitf to take a look

@akosyakov
Copy link
Member

@JPinkney it is fine with me to merge

@benoitf
Copy link
Contributor

benoitf commented Oct 9, 2019

you can merge

@JPinkney JPinkney merged commit ef08840 into eclipse-theia:master Oct 10, 2019
@JPinkney JPinkney deleted the pass-plugin-info-output-registry branch October 10, 2019 12:08
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.

4 participants