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

[Code] refactor launcher code, add unit tests #36863

Merged
merged 1 commit into from
May 24, 2019

Conversation

spacedragon
Copy link
Contributor

Summary

Refactor launcher code, add unit tests
https://github.com/elastic/code/issues/1182
https://github.com/elastic/code/issues/1167

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@mw-ding mw-ding left a comment

Choose a reason for hiding this comment

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

some minor comments inline.

* you may not use this file except in compliance with the Elastic License.
*/
/* eslint-disable */
// This file is used in the test, using subprocess.fork to load a module directly, so this module can not be typescript
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

server.close();
this.eventEmitter.emit('connect');
socket.on('close', () => this.onSocketClosed());
if (!this.connectingPromise) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: so this tries to dedup the connection promises, right? can we leave some comment on here on what issue it solves.

}
const log: Logger = this.loggerFactory.getLogger(['code', `ts@${this.targetHost}:${port}`]);
const proxy = new LanguageServerProxy(port, this.targetHost, log, this.options.lsp);
return 2089;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe a const at the beginning of this class instead of a hard-wired value.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mw-ding
Copy link
Contributor

mw-ding commented May 23, 2019

image

Tested locally on java language server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants