Repository: palantir/python-language-server
Pull request #66 (2017-06-28 09:24:17)

Config object passed to hooks
Pass a config object to hooks that wraps the plugin manager. This allows plugins to register themselves conditionally on other plugins, or to register class-based plugins which can then store state.

Also some linting fixes.

Follow-ups in #65


Patch:
diff --git a/.pylintrc b/.pylintrc
new file mode 100644
index 0000000..c86feba
--- /dev/null
+++ b/.pylintrc
@@ -0,0 +1,26 @@
+[FORMAT]
+
+max-line-length = 120
+
+[MESSAGES CONTROL]
+
+enable =
+    useless-suppression
+
+disable =
+    duplicate-code,
+    invalid-name,
+    fixme,
+    missing-docstring,
+    protected-access,
+    too-few-public-methods,
+    too-many-arguments,
+    too-many-instance-attributes
+
+[REPORTS]
+
+reports = no
+
+[TYPECHECK]
+
+generated-members = pyls_*
diff --git a/pyls/config.py b/pyls/config.py
new file mode 100644
index 0000000..26a05e6
--- /dev/null
+++ b/pyls/config.py
@@ -0,0 +1,77 @@
+# Copyright 2017 Palantir Technologies, Inc.
+from configparser import RawConfigParser
+import logging
+import os
+import pluggy
+
+from . import hookspecs, PYLS
+
+log = logging.getLogger(__name__)
+
+
+class Config(object):
+
+    def __init__(self, root_path, init_opts):
+        self._root_path = root_path
+        self._init_opts = init_opts
+
+        self._pm = pluggy.PluginManager(PYLS)
+        self._pm.trace.root.setwriter(log.debug)
+        self._pm.enable_tracing()
+        self._pm.add_hookspecs(hookspecs)
+        self._pm.load_setuptools_entrypoints(PYLS)
+
+    @property
+    def plugin_manager(self):
+        return self._pm
+
+    @property
+    def init_opts(self):
+        return self._init_opts
+
+    @property
+    def root_path(self):
+        return self._root_path
+
+    def find_parents(self, path, names):
+        return find_parents(self.root_path, path, names)
+
+
+def build_config(key, config_files):
+    """Parse configuration from the given files for the given key."""
+    config = RawConfigParser()
+
+    if not config_files:
+        # If no config files match, we can't do much
+        return {}
+
+    # Find out which section header is used, pep8 or pycodestyle
+    files_read = config.read(config_files)
+    log.debug("Loaded configuration from %s", files_read)
+
+    if config.has_section(key):
+        return {k: v for k, v in config.items(key)}
+
+    return {}
+
+
+def find_parents(root, path, names):
+    """Find files matching the given names relative to the given path.
+
+    Args:
+        path (str): The path to start searching up from
+        names (List[str]): The file/directory names to look for
+        root (str): The directory at which to stop recursing upwards.
+
+    Note:
+        The path MUST be within the root.
+    """
+    if not os.path.commonprefix((root, path)):
+        raise ValueError("Path %s not in %s" % (path, root))
+    curdir = os.path.dirname(path)
+
+    while curdir != os.path.dirname(root) and curdir != '/':
+        existing = list(filter(os.path.exists, [os.path.join(curdir, n) for n in names]))
+        if existing:
+            return existing
+        curdir = os.path.dirname(curdir)
diff --git a/pyls/hookspecs.py b/pyls/hookspecs.py
index fb6a88d..14c2e52 100644
--- a/pyls/hookspecs.py
+++ b/pyls/hookspecs.py
@@ -4,17 +4,17 @@ from pyls import hookspec
 
 
 @hookspec
-def pyls_code_actions(workspace, document, range, context):
+def pyls_code_actions(config, workspace, document, range, context):
     pass
 
 
 @hookspec
-def pyls_code_lens(workspace, document):
+def pyls_code_lens(config, workspace, document):
     pass
 
 
 @hookspec
-def pyls_commands(workspace):
+def pyls_commands(config, workspace):
     """The list of command strings supported by the server.
 
     Returns:
@@ -23,60 +23,65 @@ def pyls_commands(workspace):
 
 
 @hookspec
-def pyls_completions(workspace, document, position):
+def pyls_completions(config, workspace, document, position):
     pass
 
 
 @hookspec
-def pyls_definitions(workspace, document, position):
+def pyls_definitions(config, workspace, document, position):
     pass
 
 
 @hookspec
-def pyls_document_did_save(workspace, document):
+def pyls_document_did_open(config, workspace, document):
     pass
 
 
 @hookspec
-def pyls_document_symbols(workspace, document):
+def pyls_document_did_save(config, workspace, document):
+    pass
+
+
+@hookspec
+def pyls_document_symbols(config, workspace, document):
     pass
 
 
 @hookspec(firstresult=True)
-def pyls_execute_command(workspace, command, arguments):
+def pyls_execute_command(config, workspace, command, arguments):
     pass
 
 
 @hookspec(firstresult=True)
-def pyls_format_document(workspace, document):
+def pyls_format_document(config, workspace, document):
     pass
 
 
 @hookspec(firstresult=True)
-def pyls_format_range(workspace, document, range):
+def pyls_format_range(config, workspace, document, range):
     pass
 
 
 @hookspec(firstresult=True)
-def pyls_hover(workspace, document, position):
+def pyls_hover(config, workspace, document, position):
     pass
 
 
 @hookspec
-def pyls_initialize(workspace):
+def pyls_initialize(config, workspace):
     pass
 
 
 @hookspec
-def pyls_lint(workspace, document):
+def pyls_lint(config, workspace, document):
     pass
 
 
 @hookspec
-def pyls_references(workspace, document, position, exclude_declaration):
+def pyls_references(config, workspace, document, position, exclude_declaration):
     pass
 
 
 @hookspec(firstresult=True)
-def pyls_signature_help(workspace, document, position):
+def pyls_signature_help(config, workspace, document, position):
     pass
diff --git a/pyls/language_server.py b/pyls/language_server.py
index fb000fa..431c21e 100644
--- a/pyls/language_server.py
+++ b/pyls/language_server.py
@@ -3,7 +3,6 @@ import logging
 import re
 import socketserver
 from .server import JSONRPCServer
-from .workspace import Workspace
 
 log = logging.getLogger(__name__)
 
@@ -11,6 +10,8 @@ log = logging.getLogger(__name__)
 class _StreamHandlerWrapper(socketserver.StreamRequestHandler, object):
     """A wrapper class that is used to construct a custom handler class."""
 
+    delegate = None
+
     def setup(self):
         super(_StreamHandlerWrapper, self).setup()
         self.delegate = self.DELEGATE_CLASS(self.rfile, self.wfile)
@@ -74,22 +75,22 @@ class LanguageServer(MethodJSONRPCServer):
     """
 
     process_id = None
-    workspace = None  # type: Workspace
+    root_path = None
     init_opts = None
 
     def capabilities(self):
         return {}
 
-    def initialize(self):
+    def initialize(self, root_path, init_opts, process_id):
         pass
 
     def m_initialize(self, **kwargs):
         log.debug("Language server intialized with %s", kwargs)
-        self.process_id = kwargs.get('processId')
-        self.workspace = Workspace(kwargs.get('rootPath'), lang_server=self)
+        self.root_path = kwargs.get('rootPath')
         self.init_opts = kwargs.get('initializationOptions')
+        self.process_id = kwargs.get('processId')
 
-        self.initialize()
+        self.initialize(self.root_path, self.init_opts, self.process_id)
 
         # Get our capabilities
         return {'capabilities': self.capabilities()}
@@ -101,10 +102,10 @@ class LanguageServer(MethodJSONRPCServer):
         # This tends to happen when cancelling a hover request
         pass
 
-    def m_shutdown(self, **kwargs):
+    def m_shutdown(self, **_kwargs):
         self.shutdown()
 
-    def m_exit(self, **kwargs):
+    def m_exit(self, **_kwargs):
         self.shutdown()
 
 
diff --git a/pyls/plugins/pycodestyle_lint.py b/pyls/plugins/pycodestyle_lint.py
index 0e0fc6b..9f9b751 100644
--- a/pyls/plugins/pycodestyle_lint.py
+++ b/pyls/plugins/pycodestyle_lint.py
@@ -1,8 +1,7 @@
 # Copyright 2017 Palantir Technologies, Inc.
-from configparser import RawConfigParser
 import logging
 import pycodestyle
-from pyls import hookimpl
+from pyls import config as pyls_config, hookimpl
 
 log = logging.getLogger(__name__)
 
@@ -11,9 +10,15 @@ CONFIG_FILES = ['tox.ini', 'pep8.cfg', 'setup.cfg', 'pycodestyle.cfg']
 
 
 @hookimpl
-def pyls_lint(workspace, document):
+def pyls_lint(config, workspace, document):
     # Read config from all over the place
-    conf = {k.replace("-", "_"): v for k, v in _get_config(workspace, document)}
+    config_files = config.find_parents(document.path, CONFIG_FILES)
+    pycodestyle_conf = pyls_config.build_config('pycodestyle', config_files)
+    pep8_conf = pyls_config.build_config('pep8', config_files)
+
+    conf_to_use = pycodestyle_conf if pycodestyle_conf else pep8_conf
+
+    conf = {k.replace("-", "_"): v for k, v in conf_to_use.items()}
     log.debug("Got pycodestyle config: %s", conf)
 
     # Grab the pycodestyle parser and set the defaults based on the config we found
@@ -31,28 +36,6 @@ def pyls_lint(workspace, document):
     return diagnostics
 
 
-def _get_config(workspace, document):
-    """ Parse the pep8/pycodestyle config options. """
-    config = RawConfigParser()
-    config_files = workspace.find_parent_files(document.path, CONFIG_FILES)
-
-    if not config_files:
-        # If no config files match, we can't do much
-        return []
-
-    # Find out which section header is used, pep8 or pycodestyle
-    files_read = config.read(config_files)
-    log.debug("Using pycodestyle config from %s", files_read)
-
-    if config.has_section('pycodestyle'):
-        return config.items('pycodestyle')
-    if config.has_section('pep8'):
-        log.warning("The 'pep8' section is deprecated, use 'pycodestyle' instead")
-        return config.items('pep8')
-
-    return []
-
-
 class PyCodeStyleDiagnosticReport(pycodestyle.BaseReport):
 
     def __init__(self, options=None, **kwargs):
diff --git a/pyls/python_ls.py b/pyls/python_ls.py
index 1fcb2c5..0cc0e40 100644
--- a/pyls/python_ls.py
+++ b/pyls/python_ls.py
@@ -1,27 +1,17 @@
-# Copyright 2017 Palantir Technologies, Inc.
 import logging
-import pluggy
 
-from . import hookspecs, plugins, PYLS
+from . import config, lsp, plugins
 from .language_server import LanguageServer
-from .lsp import TextDocumentSyncKind
+from .workspace import Workspace
 
 log = logging.getLogger(__name__)
 
 
 class PythonLanguageServer(LanguageServer):
 
-    def __init__(self, *args, **kwargs):
-        # TODO(gatesn): In the future we may need to reconstruct the pm at runtime if configs change
-        self._pm = pluggy.PluginManager(PYLS)
-        self._pm.trace.root.setwriter(log.debug)
-        self._pm.enable_tracing()
-        self._pm.add_hookspecs(hookspecs)
-        self._pm.load_setuptools_entrypoints(PYLS)
-        for plugin in plugins.CORE_PLUGINS:
-            self._pm.register(plugin)
-        log.info("Loaded plugins: %s", self._pm.get_plugins())
-        super(PythonLanguageServer, self).__init__(*args, **kwargs)
+    _hooks = None
+    workspace = None
+    config = None
 
     def capabilities(self):
         # TODO: support incremental sync instead of full
@@ -39,76 +29,88 @@ class PythonLanguageServer(LanguageServer):
             'documentSymbolProvider': True,
             'definitionProvider': True,
             'executeCommandProvider': {
-                'commands': flatten(self._hook(self._pm.hook.pyls_commands))
+                'commands': flatten(self._hook(self._hooks.pyls_commands))
             },
             'hoverProvider': True,
             'referencesProvider': True,
             'signatureHelpProvider': {
                 'triggerCharacters': ['(', ',']
             },
-            'textDocumentSync': TextDocumentSyncKind.FULL
+            'textDocumentSync': lsp.TextDocumentSyncKind.FULL
         }
 
-    def initialize(self):
-        self._hook(self._pm.hook.pyls_initialize)
+    def initialize(self, root_path, init_opts, _process_id):
+        self.workspace = Workspace(root_path, lang_server=self)
+        self.config = config.Config(root_path, init_opts)
+
+        # Register the base set of plugins
+        # TODO(gatesn): Make these configurable in init_opts
+        for plugin in plugins.CORE_PLUGINS:
+            self.config.plugin_manager.register(plugin)
+
+        # Store a reference to the plugin manager's hook relay to keep things neat
+        self._hooks = self.config.plugin_manager.hook
+
+        self._hook(self._hooks.pyls_initialize)
 
     def _hook(self, hook, doc_uri=None, **kwargs):
         doc = self.workspace.get_document(doc_uri) if doc_uri else None
-        return hook(workspace=self.workspace, document=doc, **kwargs)
+        return hook(config=self.config, workspace=self.workspace, document=doc, **kwargs)
 
     def code_actions(self, doc_uri, range, context):
-        return flatten(self._hook(self._pm.hook.pyls_code_actions, doc_uri, range=range, context=context))
+        return flatten(self._hook(self._hooks.pyls_code_actions, doc_uri, range=range, context=context))
 
     def code_lens(self, doc_uri):
-        return flatten(self._hook(self._pm.hook.pyls_code_lens, doc_uri))
+        return flatten(self._hook(self._hooks.pyls_code_lens, doc_uri))
 
     def completions(self, doc_uri, position):
-        completions = self._hook(self._pm.hook.pyls_completions, doc_uri, position=position)
+        completions = self._hook(self._hooks.pyls_completions, doc_uri, position=position)
         return {
             'isIncomplete': False,
             'items': flatten(completions)
         }
 
     def definitions(self, doc_uri, position):
-        return flatten(self._hook(self._pm.hook.pyls_definitions, doc_uri, position=position))
+        return flatten(self._hook(self._hooks.pyls_definitions, doc_uri, position=position))
 
     def document_symbols(self, doc_uri):
-        return flatten(self._hook(self._pm.hook.pyls_definitions, doc_uri))
+        return flatten(self._hook(self._hooks.pyls_document_symbols, doc_uri))
 
     def execute_command(self, command, arguments):
-        return self._hook(self._pm.hook.pyls_execute_command, command=command, arguments=arguments)
+        return self._hook(self._hooks.pyls_execute_command, command=command, arguments=arguments)
 
     def format_document(self, doc_uri):
-        return self._hook(self._pm.hook.pyls_definitions, doc_uri)
+        return self._hook(self._hooks.pyls_definitions, doc_uri)
 
     def format_range(self, doc_uri, range):
-        return self._hook(self._pm.hook.pyls_format_range, doc_uri, range=range)
+        return self._hook(self._hooks.pyls_format_range, doc_uri, range=range)
 
     def hover(self, doc_uri, position):
-        return self._hook(self._pm.hook.pyls_hover, doc_uri, position=position) or {'contents': ''}
+        return self._hook(self._hooks.pyls_hover, doc_uri, position=position) or {'contents': ''}
 
     def lint(self, doc_uri):
         self.workspace.publish_diagnostics(doc_uri, flatten(self._hook(
-            self._pm.hook.pyls_lint, doc_uri
+            self._hooks.pyls_lint, doc_uri
         )))
 
     def references(self, doc_uri, position, exclude_declaration):
         return flatten(self._hook(
-            self._pm.hook.pyls_references, doc_uri, position=position,
+            self._hooks.pyls_references, doc_uri, position=position,
             exclude_declaration=exclude_declaration
         ))
 
     def signature_help(self, doc_uri, position):
-        return self._hook(self._pm.hook.pyls_signature_help, doc_uri, position=position)
+        return self._hook(self._hooks.pyls_signature_help, doc_uri, position=position)
 
-    def m_text_document__did_close(self, textDocument=None, **kwargs):
+    def m_text_document__did_close(self, textDocument=None, **_kwargs):
         self.workspace.rm_document(textDocument['uri'])
 
-    def m_text_document__did_open(self, textDocument=None, **kwargs):
+    def m_text_document__did_open(self, textDocument=None, **_kwargs):
         self.workspace.put_document(textDocument['uri'], textDocument['text'], version=textDocument.get('version'))
+        self._hook(self._hooks.pyls_document_did_open, textDocument['uri'])
         self.lint(textDocument['uri'])
 
-    def m_text_document__did_change(self, contentChanges=None, textDocument=None, **kwargs):
+    def m_text_document__did_change(self, contentChanges=None, textDocument=None, **_kwargs):
         # Since we're using a FULL document sync, there is only one change containing the whole file
         # TODO: debounce, or should this be someone else's responsibility? Probably
         self.workspace.put_document(
@@ -116,43 +118,43 @@ class PythonLanguageServer(LanguageServer):
         )
         self.lint(textDocument['uri'])
 
-    def m_text_document__did_save(self, textDocument=None, **kwargs):
+    def m_text_document__did_save(self, textDocument=None, **_kwargs):
         self.lint(textDocument['uri'])
 
-    def m_text_document__code_action(self, textDocument=None, range=None, context=None, **kwargs):
+    def m_text_document__code_action(self, textDocument=None, range=None, context=None, **_kwargs):
         return self.code_actions(textDocument['uri'], range, context)
 
-    def m_text_document__code_lens(self, textDocument=None, **kwargs):
+    def m_text_document__code_lens(self, textDocument=None, **_kwargs):
         return self.code_lens(textDocument['uri'])
 
-    def m_text_document__completion(self, textDocument=None, position=None, **kwargs):
+    def m_text_document__completion(self, textDocument=None, position=None, **_kwargs):
         return self.completions(textDocument['uri'], position)
 
-    def m_text_document__definition(self, textDocument=None, position=None, **kwarg):
+    def m_text_document__definition(self, textDocument=None, position=None, **_kwargs):
         return self.definitions(textDocument['uri'], position)
 
-    def m_text_document__hover(self, textDocument=None, position=None, **kwargs):
+    def m_text_document__hover(self, textDocument=None, position=None, **_kwargs):
         return self.hover(textDocument['uri'], position)
 
-    def m_text_document__document_symbol(self, textDocument=None, **kwargs):
+    def m_text_document__document_symbol(self, textDocument=None, **_kwargs):
         return self.document_symbols(textDocument['uri'])
 
-    def m_text_document__formatting(self, textDocument=None, options=None, **kwargs):
+    def m_text_document__formatting(self, textDocument=None, options=None, **_kwargs):
         # For now we're ignoring formatting options.
         return self.format_document(textDocument['uri'])
 
-    def m_text_document__range_formatting(self, textDocument=None, range=None, options=None, **kwargs):
+    def m_text_document__range_formatting(self, textDocument=None, range=None, options=None, **_kwargs):
         # Again, we'll ignore formatting options for now.
         return self.format_range(textDocument['uri'], range)
 
-    def m_text_document__references(self, textDocument=None, position=None, context=None, **kwargs):
+    def m_text_document__references(self, textDocument=None, position=None, context=None, **_kwargs):
         exclude_declaration = not context['includeDeclaration']
         return self.references(textDocument['uri'], position, exclude_declaration)
 
-    def m_text_document__signature_help(self, textDocument=None, position=None, **kwargs):
+    def m_text_document__signature_help(self, textDocument=None, position=None, **_kwargs):
         return self.signature_help(textDocument['uri'], position)
 
-    def m_workspace__did_change_watched_files(self, **kwargs):
+    def m_workspace__did_change_watched_files(self, **_kwargs):
         pass
 
     def m_workspace__execute_command(self, command=None, arguments=None):
diff --git a/pyls/workspace.py b/pyls/workspace.py
index e648cf4..f7a5b06 100644
--- a/pyls/workspace.py
+++ b/pyls/workspace.py
@@ -7,7 +7,7 @@ from urllib.parse import urlparse, urlunparse
 
 import jedi
 
-from pyls.lsp import MessageType
+from . import config, lsp
 
 log = logging.getLogger(__name__)
 
@@ -52,7 +52,7 @@ class Workspace(object):
         params = {'uri': doc_uri, 'diagnostics': diagnostics}
         self._lang_server.notify(self.M_PUBLISH_DIAGNOSTICS, params)
 
-    def show_message(self, message, msg_type=MessageType.Info):
+    def show_message(self, message, msg_type=lsp.MessageType.Info):
         params = {'type': msg_type, 'message': message}
         self._lang_server.notify(self.M_SHOW_MESSAGE, params)
 
@@ -62,24 +62,11 @@ class Workspace(object):
         Since the workspace root may not be the root of the Python project we instead
         append the closest parent directory containing a setup.py file.
         """
-        files = self.find_parent_files(path, ['setup.py']) or []
+        files = config.find_parents(self.root, path, ['setup.py']) or []
         path = [os.path.dirname(setup_py) for setup_py in files]
         path.extend(sys.path)
         return path
 
-    def find_parent_files(self, path, names):
-        """Find files matching the given names relative to the given
-        document looking in parent directories until one is found """
-        self._check_in_workspace(path)
-        curdir = os.path.dirname(path)
-
-        while curdir != os.path.dirname(self.root) and curdir != '/':
-            existing = list(
-                filter(os.path.exists, [os.path.join(curdir, n) for n in names]))
-            if existing:
-                return existing
-            curdir = os.path.dirname(curdir)
-
     def _check_in_workspace(self, path):
         if not os.path.commonprefix((self.root, path)):
             raise ValueError("Document %s not in workspace %s" % (path, self.root))


Test patch:
diff --git a/test/fixtures.py b/test/fixtures.py
index e386a60..0418a0a 100644
--- a/test/fixtures.py
+++ b/test/fixtures.py
@@ -1,6 +1,7 @@
 # Copyright 2017 Palantir Technologies, Inc.
 import os
 import pytest
+from pyls.config import Config
 from pyls.python_ls import PythonLanguageServer
 from pyls.workspace import Workspace
 from io import StringIO
@@ -24,5 +25,11 @@ def pyls(tmpdir):
 
 @pytest.fixture
 def workspace():
-    """ Return a workspace """
+    """Return a workspace."""
     return Workspace(os.path.dirname(__file__))
+
+
+@pytest.fixture
+def config(workspace):
+    """Return a config object."""
+    return Config(workspace.root, {})
diff --git a/test/plugins/test_lint.py b/test/plugins/test_lint.py
index ecedbed..2a5880f 100644
--- a/test/plugins/test_lint.py
+++ b/test/plugins/test_lint.py
@@ -2,6 +2,7 @@
 import os
 import shutil
 import tempfile
+from pyls.config import Config
 from pyls.workspace import Document, Workspace
 from pyls.plugins import pycodestyle_lint, pyflakes_lint
 
@@ -19,9 +20,9 @@ DOC_SYNTAX_ERR = """def hello()
 """
 
 
-def test_pycodestyle(workspace):
+def test_pycodestyle(config, workspace):
     doc = Document(DOC_URI, DOC)
-    diags = pycodestyle_lint.pyls_lint(workspace, doc)
+    diags = pycodestyle_lint.pyls_lint(config, workspace, doc)
 
     assert all([d['source'] == 'pycodestyle' for d in diags])
 
@@ -54,10 +55,11 @@ def test_pycodestyle_config():
     doc_uri = 'file://' + tmp + '/' + 'test.py'
     workspace.put_document(doc_uri, DOC)
     doc = workspace.get_document(doc_uri)
+    config = Config(workspace.root, {})
 
     # Make sure we get a warning for 'indentation contains tabs'
-    diags = pycodestyle_lint.pyls_lint(workspace, doc)
-    assert len([d for d in diags if d['code'] == 'W191']) > 0
+    diags = pycodestyle_lint.pyls_lint(config, workspace, doc)
+    assert [d for d in diags if d['code'] == 'W191']
 
     content = {
         'setup.cfg': ('[pycodestyle]\nignore = W191', True),
@@ -71,7 +73,7 @@ def test_pycodestyle_config():
             f.write(content)
 
         # And make sure we don't get any warnings
-        diags = pycodestyle_lint.pyls_lint(workspace, doc)
+        diags = pycodestyle_lint.pyls_lint(config, workspace, doc)
         assert len([d for d in diags if d['code'] == 'W191']) == 0 if working else 1
 
         os.unlink(os.path.join(tmp, conf_file))