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

Cleanup extHostTerminalService.ts #84404

Merged
merged 3 commits into from Nov 17, 2019

Conversation

@solomatov
Copy link
Contributor

solomatov commented Nov 10, 2019

  • Remove code having no effect in _getTerminalByIdEventually
  • Remove unused method
  • Replace 200 with 2 * EXT_HOST_CREATION_DELAY

Please, correct me if I made something wrongly.

solomatov added 2 commits Nov 10, 2019
remove dead code
@solomatov solomatov closed this Nov 10, 2019
@solomatov solomatov deleted the solomatov:cleanup_ext_host_terminals branch Nov 10, 2019
@solomatov solomatov restored the solomatov:cleanup_ext_host_terminals branch Nov 10, 2019
@solomatov solomatov reopened this Nov 10, 2019
@solomatov

This comment has been minimized.

Copy link
Contributor Author

solomatov commented Nov 15, 2019

@Tyriar Any updates on this?

@Tyriar
Tyriar approved these changes Nov 17, 2019
} else {
this._getTerminalPromises[id].then(c => {
return this._createGetTerminalPromise(id, retries);
});
Comment on lines -553 to -556

This comment has been minimized.

Copy link
@Tyriar

Tyriar Nov 17, 2019

Member

I think the purpose of this code was to handle the case where the promise this._getTerminalPromises[id] resolves to undefined. It definitely doesn't work currently but that might be a case that's still an issue.

This comment has been minimized.

Copy link
@solomatov

solomatov Nov 18, 2019

Author Contributor

@Tyriar But it doesn't return anything, so no effect. Or was there any side effect of invoking _getTerminalByIdEventually in code before some of the changes?

This comment has been minimized.

Copy link
@Tyriar

Tyriar Nov 18, 2019

Member

Yeah it definitely didn't work so it's fine to remove, I think we're still missing that case though. Ideally the resolve function would be kept around, similar to how id promise works on BaseExtHostTerminal:

protected _idPromise: Promise<number>;
private _idPromiseComplete: ((value: number) => any) | undefined;

@Tyriar Tyriar added this to the November 2019 milestone Nov 17, 2019
@Tyriar Tyriar merged commit a8be2f9 into microsoft:master Nov 17, 2019
4 of 5 checks passed
4 of 5 checks passed
linux
Details
windows
Details
darwin
Details
VS Code in progress
Details
license/cla All CLA requirements met.
Details
@solomatov solomatov deleted the solomatov:cleanup_ext_host_terminals branch Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.