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

Refactor client #118

Closed
wants to merge 2 commits into from
Closed

Refactor client #118

wants to merge 2 commits into from

Conversation

mbana
Copy link

@mbana mbana commented Oct 29, 2016

Refactoring the client a bit

  • moving classes to separate files.
  • created classes for the Converters.

We're also doing a WebSocketReader. If my employer is happy to push the changes back, I'll create a PR for that as well. That said, the client lib will be structured differently though so as to only allow the importer to choose a specific set of transports required.

Many thanks,

mbana added 2 commits October 28, 2016 15:30
* move converters - code to protocol and protocol to code - to converts folder.
@msftclas
Copy link

Hi @mbana, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

1 similar comment
@msftclas
Copy link

Hi @mbana, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@akosyakov
Copy link
Contributor

@mbana Is it about Monaco integration: #88? How does it help? I see that converters still depend on vscode types: https://github.com/Microsoft/vscode-languageserver-node/pull/118/files#diff-e05feeb2a7817f9fe64dd047e2c12b00R1.

Is not having WebSocketReader and WebSocketWriter enough to change transport? Why not?

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Nov 22, 2016
@dbaeumer
Copy link
Member

@mbana I agree with @akosyakov: why does this help and what is the improvement. The client already exports both converters as properties.

@mbana
Copy link
Author

mbana commented Nov 22, 2016

i was trying to understand the structure of the various projects and along the way i started to move classes etc. to separate files. i guess it saves time by having most of the implementation in one file?

vscode-languageserver-types: not sure i understand. why is this an issue? the package seems mostly interfaces... please explain.

@dbaeumer
Copy link
Member

@mbana What do you mean by

vscode-languageserver-types: not sure i understand. why is this an issue? the package seems mostly interfaces... please explain.

@mbana
Copy link
Author

mbana commented Nov 25, 2016

i wasn't sure what @akosyakov meant.

that said, why this an issue? vscode-languageserver-types doesn't include vscode so that's fine. it's only vscode-languageclient that does which is understandable - it marshalls to and fo a LSP. couldn't every LSP client need to do this?

@mbana mbana closed this Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info-needed Issue requires more information from poster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants