langserver.py: Update to use jsonrpc #32

ksdme · 2018-03-26T14:00:13Z

Updates coala-vs-code to use jsonrpc module from python-language-server (palantir/python-language-server#32).

Closes #27

gaocegege · 2018-03-27T06:52:46Z

Thanks for your contribution! Could you please fix the test failures?

ksdme · 2018-03-27T06:55:18Z

I am already working on it.

The jsonrpc behave tests are updated. The test has remained unchanged. Related to coala#27, coala#32

gaocegege · 2018-03-28T07:43:49Z

Please follow the contribution guide here #31 (comment)

The jsonrpc behave tests have been updated. The scope of the test has remained unchanged. Related to coala#27, coala#32

The behave tests have been updated to reflect the new jsonrpc handling mechanism. The scope of the tests remains unchanged. Related to coala#27, coala#32

ksdme · 2018-03-28T10:15:40Z

I have only updated the tests as of now, I am working on improving the coverage and fix a minor logging issue during running testing.

jayvdb · 2018-03-29T13:49:49Z

README.md

@@ -13,7 +13,7 @@ A visual studio code plugin working via [Language Server Protocol (LSP)](https:/

 You'll need python version 3.5 or greater, run `pip3 install -r requirements.txt` to install the requirements, and run `python3 langserver-python.py --mode=tcp --addr=2087` to start a local languager server listening at port 2087.

-Then you should update the `./vscode-client/src/extension.ts` to make client in TCP mode.
+Then you should update the `./vscode-client/src/extension.ts` to make client in TCP mode. 


discard this change. create a newcomer bug about adding style linting rules for this file.

jayvdb · 2018-03-29T13:50:11Z

README.md

@@ -24,7 +24,7 @@ export function activate(context: ExtensionContext) {
 }
 ```

-To try it in [Visual Studio Code](https://code.visualstudio.com), open ./vscode-client in VS Code and turn to debug view, launch the extension.
+To try it in [Visual Studio Code](https://code.visualstudio.com), open ./vscode-client in VS Code and turn to debug view, launch the extension. If the langserver fails try disabling the GitCommitBear.


Create a newcomer issue about this, and remove it from your PR. it has nothing to do with jsonrpc.

Also a change like this should be done by adding a new line, so the origin of existing line doesnt change unnecessarily.

jayvdb · 2018-03-29T13:50:27Z

coala_langserver/diagnostic.py


+log = logging.getLogger(__name__)


call this logger

jayvdb · 2018-03-29T13:52:03Z

coala_langserver/langserver.py

+    def start(self):
+        self._jsonrpc_stream_reader.listen(self._endpoint.consume)
+
+    # def handle(self, _id, request):


we dont want commented out code.

If there is extra stuff to be done, create an issue after this is merged

A clean up issue, I just wanted to have a reference to the previous code to cross verify.

jayvdb · 2018-03-29T13:54:43Z

coala_langserver/langserver.py

        params = {
            'uri': 'file://{0}'.format(path),
-            'diagnostics': _diagnostics,
+            'diagnostics': [] if diagnostics is None else diagnostics,


this seems to be a pointless change

jayvdb · 2018-03-29T14:11:37Z

coala_langserver/jsonrpc/dispatchers.py

@@ -0,0 +1,35 @@
+# Copyright 2018 Palantir Technologies, Inc.


I would prefer that we did not copy other code into the repository. For example, I can not easily check to see if you modified a line from the origin.

While there are some additional dependencies at https://github.com/palantir/python-language-server/blob/develop/setup.py#L34 , could we not require that it is installed, and then use the jsonrpc they provide? If that is not possible, maybe we can coordinate with @palantir to split that out into a sharable library.

This tool is still a prototype, so it can have additional install steps , and keeping the origin of source code clear.

It also means that additional tests for the jsonrpc would be contributed to https://github.com/palantir/python-language-server , so everyone benefits.

I will follow your recommendations, python-lang-server will be added as a dependency.

jayvdb · 2018-03-29T14:14:52Z

tests/server.features/steps/langserver_steps.py


 @given('the LangServer instance')
 def step_impl(context):
-    context.f = tempfile.TemporaryFile()
-    context.langServer = LangServer(conn=TCPReadWriter(context.f, context.f))
+    context.file = tempfile.TemporaryFile()


why does it need renaming from f to file? It causes all sorts of other unnecessary changes to appear in this PR, slowing down the review.

is that related to the issue that this PR is intending to solve.

'f' didn't seem to be a semantic name. My intention was to improve readability, retaining such occurrences could indicate tolerance for those practices and could even mean encouragement.

I think the re-naming is justified, do you still recommend this particular change?

Of course I do. it is not related to using jsonrpc. Create a new PR for other changes you wish to make.

jayvdb · 2018-03-29T14:17:39Z

coala_langserver/langserver.py

+    """
+    PY3K = sys.version_info >= (3, 0)
+
+    if PY3K:


jayvdb · 2018-03-29T14:28:25Z

coala_langserver/langserver.py

+        server = socketserver.TCPServer((bind_addr, port), wrapper_class)
+    except Exception as e:
+        log.fatal("Exception: %s", str(e))
+        exit(1)


sys.exit?

jayvdb · 2018-03-29T14:32:15Z

coala_langserver/langserver.py


-from .jsonrpc import JSONRPC2Connection, ReadWriter, TCPReadWriter
-from .log import log


removing this (and other logging enhancements) should be done as a separate PR. I've assigned #36 to you, as you've already done a lot of this.

The jsonrpc behave tests have been updated. The scope of the test has remained unchanged. Related to coala#27, coala#32

The behave tests have been updated to reflect the new jsonrpc handling mechanism. The scope of the tests remains unchanged. Related to coala#27, coala#32

jayvdb · 2018-04-04T05:57:13Z

coala_langserver/langserver.py

@@ -108,7 +108,11 @@ def start_tcp_lang_server(bind_addr, port, handler_class):
        {'DELEGATE_CLASS': handler_class}
    )

-    server = socketserver.TCPServer((bind_addr, port), wrapper_class)
+    try:


This change should be a new PR.

Ideally create a new issue showing why this change is needed.

jayvdb · 2018-04-04T05:59:33Z

coala_langserver/langserver.py

-        # s = LangServer(conn=ReadWriter(sys.stdin, sys.stdout))
-        # s.listen()
-        pass
+        stdin, stdout = _binary_stdio()


move this code out to a PR for #48

jayvdb · 2018-04-04T06:04:33Z

As far as I can see, after removing all the unrelated features, there should only be one commit in this PR. fwiw, updating tests should be part of the atomic change of modifying the code.

gaocegege · 2018-04-04T06:40:12Z

@ksdme Thanks for your contribution! I think we could try to add some tests.

gitmate-bot · 2018-04-05T06:16:35Z

coala_langserver/langserver.py

-class ThreadingTCPServer(socketserver.ThreadingMixIn, socketserver.TCPServer):
-    pass
+class _StreamHandlerWrapper(socketserver.StreamRequestHandler, object):
+    """A wrapper class that is used to construct a custom handler class."""


You do not use the preferred quotation marks.

Origin: QuotesBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp_evza00r/coala_langserver/langserver.py +++ b/tmp/tmp_evza00r/coala_langserver/langserver.py @@ -14,7 +14,7 @@ class _StreamHandlerWrapper(socketserver.StreamRequestHandler, object): - """A wrapper class that is used to construct a custom handler class.""" + '""A wrapper class that is used to construct a custom handler class.""' delegate = None

gitmate-bot · 2018-04-05T06:16:35Z

coala_langserver/langserver.py

-            self.write_response(request['id'], resp)
-
-    def serve_initialize(self, request):
+    #TODO: didChange


E265 block comment should start with '# '

Origin: PycodestyleBear (E265), Section: autopep8.

gitmate-bot · 2018-04-05T06:16:44Z

coala_langserver/langserver.py

+from pyls.jsonrpc.endpoint import Endpoint
+from pyls.jsonrpc.dispatchers import MethodDispatcher
+from pyls.jsonrpc.streams import JsonRpcStreamReader
+from pyls.jsonrpc.streams import JsonRpcStreamWriter


You do not use the preferred quotation marks.

Origin: QuotesBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp7wx9j6yx/coala_langserver/langserver.py +++ b/tmp/tmp7wx9j6yx/coala_langserver/langserver.py @@ -111,7 +111,7 @@ try: server = socketserver.TCPServer((bind_addr, port), wrapper_class) except Exception as e: - log("Fatal Exception: {}".format(e)) + log('Fatal Exception: {}'.format(e)) sys.exit(1) try: log('Serving %s on (%s, %s)', handler_class.__name__, bind_addr, port)

gitmate-bot · 2018-04-05T06:16:45Z

Comment on f253090.

Shortlog of the HEAD commit contains 60 character(s). This is 10 character(s) longer than the limit (60 > 50).

Origin: GitCommitBear, Section: commit.

gitmate-bot · 2018-04-05T06:16:57Z

Comment on 45df0f8, file coala_langserver/langserver.py, line 126.

Line is longer than allowed. (85 > 79)

Origin: LineLengthBear, Section: linelength.

gitmate-bot · 2018-05-11T09:58:18Z

Comment on de0098b, file tests/server.features/steps/jsonrpc_steps.py, line 63.

Line is longer than allowed. (83 > 79)

Origin: LineLengthBear, Section: linelength.

jayvdb · 2018-05-11T10:04:15Z

tests/server.features/steps/langserver_steps.py

+            'capabilities': {},
+        },
+        'id': 1,
+        'jsonrpc': '2.0'


trailing comma

jayvdb · 2018-05-11T10:04:30Z

tests/server.features/steps/langserver_steps.py

+            'capabilities': {},
+        },
+        'id': 1,
+        'jsonrpc': '2.0'


trailing comma

still not done?

jayvdb · 2018-05-11T10:05:23Z

coala_langserver/langserver.py

+    wrapper_class = type(
+        handler_class.__name__ + 'Handler',
+        (_StreamHandlerWrapper,),
+        {'DELEGATE_CLASS': handler_class}


trailing comma ; not likely to be needed any time soon, but likely doesnt hurt

jayvdb · 2018-05-11T10:10:42Z

coala_langserver/langserver.py

+        log('Fatal Exception: {}'.format(e))
+        sys.exit(1)
+    try:
+        log('Serving {} on ({}, {})'.format(


log isnt useful inside the try.

but, it is a new log call, and redundant due to existing Accepting TCP connections .. log

I have to retain this log compared to the other one because the tests watch this log call to trigger sending requests.

jayvdb · 2018-05-11T10:11:02Z

coala_langserver/langserver.py

+
+@enforce_signature
+def start_io_lang_server(handler_class: LangServer, rstream, wstream):
+    log('Starting {} IO language server'.format(handler_class.__name__))


new log call, also redundant.

For the reasons mentioned in the previous change request and maintaining uniformity I think I'll have to retain this log and remove the other redundant log call.

gitmate-bot · 2018-05-11T10:47:10Z

Comment on de0098b, file tests/server.features/steps/jsonrpc_steps.py, line 63.

The code does not comply to PEP8.

Origin: PEP8Bear, Section: autopep8.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpc942a8vq/tests/server.features/steps/jsonrpc_steps.py
+++ b/tmp/tmpc942a8vq/tests/server.features/steps/jsonrpc_steps.py
@@ -60,7 +60,8 @@
 @given('the JSONRPC2Connection instance')
 def step_impl(context):
     context.f = tempfile.TemporaryFile()
-    context.jsonConn = JSONRPC2Connection(conn=TCPReadWriter(context.f, context.f))
+    context.jsonConn = JSONRPC2Connection(
+        conn=TCPReadWriter(context.f, context.f))
 
 
 @when('I write a request to the JSONRPC2Connection with id')

gitmate-bot · 2018-05-11T10:47:11Z

Comment on de0098b, file tests/server.features/steps/jsonrpc_steps.py, line 10.

E501 line too long (82 > 79 characters)

Origin: PycodestyleBear (E501), Section: autopep8.

gitmate-bot · 2018-05-11T10:47:11Z

Comment on de0098b, file tests/server.features/steps/jsonrpc_steps.py, line 63.

E501 line too long (83 > 79 characters)

Origin: PycodestyleBear (E501), Section: autopep8.

gitmate-bot · 2018-05-11T10:47:12Z

Comment on de0098b, file tests/server.features/steps/jsonrpc_steps.py, line 10.

Line is longer than allowed. (82 > 79)

Origin: LineLengthBear, Section: linelength.

gitmate-bot · 2018-05-11T10:47:13Z

Comment on de0098b, file tests/server.features/steps/jsonrpc_steps.py, line 63.

Line is longer than allowed. (83 > 79)

Origin: LineLengthBear, Section: linelength.

jayvdb · 2018-05-11T11:34:45Z

coala_langserver/langserver.py

+    wrapper_class = type(
+        handler_class.__name__ + 'Handler',
+        (_StreamHandlerWrapper,),
+        {'DELEGATE_CLASS': handler_class, }


no, a trailing comma is for allowing new additions without modifications to exiting lines. hence this , is not a trailing comma.

{'DELEGATE_CLASS': handler_class},

jayvdb · 2018-05-11T11:36:12Z

coala_langserver/langserver.py

+
+@enforce_signature
+def start_io_lang_server(handler_class: LangServer, rstream, wstream):
+    log('Starting {} IO language server'.format(handler_class.__name__))


jayvdb · 2018-05-11T11:38:53Z

tests/server.features/steps/langserver_steps.py

+            'capabilities': {},
+        },
+        'id': 1,
+        'jsonrpc': '2.0'


still not done?

jayvdb · 2018-05-12T06:45:44Z

coala_langserver/langserver.py

+    def start(self):
+        self._jsonrpc_stream_reader.listen(self._endpoint.consume)
+
+    def m_initialize(self, rootUri=None, rootPath=None, **kargs):


def m_initialize(self, **params)

then nothing below needs to be modified, except removal of params = request['params']

sorry I didnt notice this earlier.

this helps show the similarities of the two jsonrpc's , making differences more clear

jayvdb · 2018-05-12T06:46:11Z

coala_langserver/langserver.py

        return {
            'capabilities': {
                'textDocumentSync': 1
            }
        }

-    def serve_did_save(self, request):
+    def m_text_document__did_save(self, textDocument=None, **kargs):


likewise use **params to avoid changes below

This updates the project to use jsonrpc module from python-language-server. Closes coala#27

jayvdb · 2018-05-12T07:19:46Z

ack bd12657

jayvdb · 2018-05-12T07:20:05Z

ack de0098b

jayvdb · 2018-05-12T07:20:38Z

@gitmate-bot ff

gitmate-bot · 2018-05-12T07:20:40Z

Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently ⚠️

gitmate-bot · 2018-05-12T07:20:53Z

Automated fastforward with GitMate.io was successful! 🎉

gaocegege · 2018-05-12T07:21:52Z

🎉

ksdme added a commit to ksdme/coala-vs-code that referenced this pull request Mar 28, 2018

Updates tests to json-rpc

77a828a

The jsonrpc behave tests are updated. The test has remained unchanged. Related to coala#27, coala#32

ksdme force-pushed the jsonrpc-dev branch from 77a828a to 63b9f75 Compare March 28, 2018 10:08

ksdme added a commit to ksdme/coala-vs-code that referenced this pull request Mar 28, 2018

Updates tests to jsonrpc

59605c2

The jsonrpc behave tests have been updated. The scope of the test has remained unchanged. Related to coala#27, coala#32

jayvdb suggested changes Mar 29, 2018

View reviewed changes

jayvdb added the process/wip label Mar 29, 2018

ksdme force-pushed the jsonrpc-dev branch from 63b9f75 to b3b46d3 Compare April 3, 2018 12:12

ksdme added a commit to ksdme/coala-vs-code that referenced this pull request Apr 3, 2018

Updates tests to jsonrpc

601463d

The jsonrpc behave tests have been updated. The scope of the test has remained unchanged. Related to coala#27, coala#32

ksdme added a commit to ksdme/coala-vs-code that referenced this pull request Apr 3, 2018

langserver_steps.py: Updates behave tests

b3b46d3

The behave tests have been updated to reflect the new jsonrpc handling mechanism. The scope of the tests remains unchanged. Related to coala#27, coala#32

ksdme force-pushed the jsonrpc-dev branch from b3b46d3 to 9426388 Compare April 4, 2018 03:23

jayvdb reviewed Apr 4, 2018

View reviewed changes

ksdme force-pushed the jsonrpc-dev branch from 9426388 to 9248c90 Compare April 5, 2018 06:16

gitmate-bot added process/pending review size/L difficulty/low importance/low and removed process/wip labels Apr 5, 2018

gitmate-bot assigned ksdme Apr 5, 2018

gitmate-bot reviewed Apr 5, 2018

View reviewed changes

jayvdb suggested changes May 11, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels May 11, 2018

ksdme force-pushed the jsonrpc-dev branch from 2013904 to 92ad7bc Compare May 11, 2018 10:47

gitmate-bot added process/pending review and removed process/wip labels May 11, 2018

jayvdb suggested changes May 11, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels May 11, 2018

ksdme force-pushed the jsonrpc-dev branch from 92ad7bc to 351c97d Compare May 11, 2018 12:53

gitmate-bot added process/pending review and removed process/wip labels May 11, 2018

jayvdb suggested changes May 12, 2018

View reviewed changes

Update to jsonrpc lib

bd12657

This updates the project to use jsonrpc module from python-language-server. Closes coala#27

ksdme force-pushed the jsonrpc-dev branch from 351c97d to bd12657 Compare May 12, 2018 07:12

jayvdb approved these changes May 12, 2018

View reviewed changes

gitmate-bot merged commit bd12657 into coala:master May 12, 2018

jayvdb mentioned this pull request Jun 6, 2018

(Phase 1) Project wide refactor and generalization coala/coala-ls#2

Open

		@@ -0,0 +1,35 @@
		# Copyright 2018 Palantir Technologies, Inc.


		from .jsonrpc import JSONRPC2Connection, ReadWriter, TCPReadWriter
		from .log import log

langserver.py: Update to use jsonrpc #32

langserver.py: Update to use jsonrpc #32

Conversation

ksdme commented Mar 26, 2018 • edited Loading

gaocegege commented Mar 27, 2018

ksdme commented Mar 27, 2018

gaocegege commented Mar 28, 2018

ksdme commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb commented Apr 4, 2018

gaocegege commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitmate-bot commented Apr 5, 2018

gitmate-bot commented Apr 5, 2018

gitmate-bot commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksdme May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitmate-bot commented May 11, 2018

gitmate-bot commented May 11, 2018

gitmate-bot commented May 11, 2018

gitmate-bot commented May 11, 2018

gitmate-bot commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb commented May 12, 2018

jayvdb commented May 12, 2018

jayvdb commented May 12, 2018

gitmate-bot commented May 12, 2018

gitmate-bot commented May 12, 2018

gaocegege commented May 12, 2018

ksdme commented Mar 26, 2018 •

edited

Loading

ksdme May 11, 2018 •

edited

Loading