Run ruff format when lsp formatting is invoked (#57)

mmcshane · web-flow · commit 9ff905cbd2f3 · 2023-11-25T13:43:56.000+01:00
* Run `ruff format` when lsp formatting is invoked

Adds the Subcommand enum to indicate which `ruff` subcommand should be
executed by `run_ruff`. At this time, only `check` and `format` are
supported. As different subcommands support different parameters,
argument generation is delegated based on the specific subcommand value.

The `ruff format` subcommand does not currently organize imports and
there does not appear to be a way to convince it to do so. Until a
unified command exists the approach taken here is to format and then
make a second run of `ruff check` that _only_ performs import
formatting.

* Preserve compatibility with `format` settings

Codes listed in this setting should be included in fixes performed as
part of a formatting pass.

* Make import sorting opt-in
diff --git a/README.md b/README.md
@@ -78,7 +78,7 @@ the valid configuration keys:
  - `pylsp.plugins.ruff.perFileIgnores`: File-specific error codes to be ignored.
  - `pylsp.plugins.ruff.select`: List of error codes to enable.
  - `pylsp.plugins.ruff.extendSelect`: Same as select, but append to existing error codes.
- - `pylsp.plugins.ruff.format`: List of error codes to fix during formatting. The default is `["I"]`, any additional codes are appended to this list.
+ - `pylsp.plugins.ruff.format`: List of error codes to fix during formatting. Empty by default, use `["I"]` here to get import sorting as part of formatting.
  - `pylsp.plugins.ruff.unsafeFixes`: boolean that enables/disables fixes that are marked "unsafe" by `ruff`. `false` by default.
  - `pylsp.plugins.ruff.severities`: Dictionary of custom severity levels for specific codes, see [below](#custom-severities).
 
diff --git a/pylsp_ruff/plugin.py b/pylsp_ruff/plugin.py
@@ -1,3 +1,4 @@
+import enum
 import json
 import logging
 import re
@@ -45,6 +46,7 @@
     r"(?::\s?(?P<codes>([A-Z]+[0-9]+(?:[,\s]+)?)+))?"
 )
 
+
 UNNECESSITY_CODES = {
     "F401",  # `module` imported but unused
     "F504",  # % format unused named arguments
@@ -61,6 +63,29 @@
 }
 
 
+class Subcommand(str, enum.Enum):
+    CHECK = "check"
+    FORMAT = "format"
+
+    def __str__(self) -> str:
+        return self.value
+
+    def build_args(
+        self,
+        document_path: str,
+        settings: PluginSettings,
+        fix: bool = False,
+        extra_arguments: Optional[List[str]] = None,
+    ) -> List[str]:
+        if self == Subcommand.CHECK:
+            return build_check_arguments(document_path, settings, fix, extra_arguments)
+        elif self == Subcommand.FORMAT:
+            return build_format_arguments(document_path, settings, extra_arguments)
+        else:
+            logging.warn(f"subcommand without argument builder '{self}'")
+        return []
+
+
 @hookimpl
 def pylsp_settings():
     log.debug("Initializing pylsp_ruff")
@@ -103,8 +128,19 @@ def pylsp_format_document(workspace: Workspace, document: Document) -> Generator
         settings=settings, document_path=document.path, document_source=source
     )
 
+    if settings.format:
+        # A second pass through the document with `ruff check` and only the rules
+        # enabled via the format config property. This allows for things like
+        # specifying `format = ["I"]` to get import sorting as part of formatting.
+        new_text = run_ruff(
+            settings=PluginSettings(ignore=["ALL"], select=settings.format),
+            document_path=document.path,
+            document_source=new_text,
+            fix=True,
+        )
+
     # Avoid applying empty text edit
-    if new_text == source:
+    if not new_text or new_text == source:
         return
 
     range = Range(
@@ -399,6 +435,7 @@ def run_ruff_check(document: Document, settings: PluginSettings) -> List[RuffChe
         document_path=document.path,
         document_source=document.source,
         settings=settings,
+        subcommand=Subcommand.CHECK,
     )
     try:
         result = json.loads(result)
@@ -422,26 +459,19 @@ def run_ruff_format(
     document_path: str,
     document_source: str,
 ) -> str:
-    fixable_codes = ["I"]
-    if settings.format:
-        fixable_codes.extend(settings.format)
-    extra_arguments = [
-        f"--fixable={','.join(fixable_codes)}",
-    ]
-    result = run_ruff(
+    return run_ruff(
         settings=settings,
         document_path=document_path,
         document_source=document_source,
-        fix=True,
-        extra_arguments=extra_arguments,
+        subcommand=Subcommand.FORMAT,
     )
-    return result
 
 
 def run_ruff(
     settings: PluginSettings,
     document_path: str,
     document_source: str,
+    subcommand: Subcommand = Subcommand.CHECK,
     fix: bool = False,
     extra_arguments: Optional[List[str]] = None,
 ) -> str:
@@ -457,6 +487,8 @@ def run_ruff(
     document_source : str
         Document source or to apply ruff on.
         Needed when the source differs from the file source, e.g. during formatting.
+    subcommand: Subcommand
+        The ruff subcommand to run. Default = Subcommand.CHECK.
     fix : bool
         Whether to run fix or no-fix.
     extra_arguments : List[str]
@@ -467,7 +499,8 @@ def run_ruff(
     String containing the result in json format.
     """
     executable = settings.executable
-    arguments = build_arguments(document_path, settings, fix, extra_arguments)
+
+    arguments = subcommand.build_args(document_path, settings, fix, extra_arguments)
 
     if executable is not None:
         log.debug(f"Calling {executable} with args: {arguments} on '{document_path}'")
@@ -478,7 +511,7 @@ def run_ruff(
         except Exception:
             log.error(f"Can't execute ruff with given executable '{executable}'.")
     else:
-        cmd = [sys.executable, "-m", "ruff"]
+        cmd = [sys.executable, "-m", "ruff", str(subcommand)]
         cmd.extend(arguments)
         p = Popen(cmd, stdin=PIPE, stdout=PIPE, stderr=PIPE)
     (stdout, stderr) = p.communicate(document_source.encode())
@@ -489,14 +522,14 @@ def run_ruff(
     return stdout.decode()
 
 
-def build_arguments(
+def build_check_arguments(
     document_path: str,
     settings: PluginSettings,
     fix: bool = False,
     extra_arguments: Optional[List[str]] = None,
 ) -> List[str]:
     """
-    Build arguments for ruff.
+    Build arguments for ruff check.
 
     Parameters
     ----------
@@ -569,6 +602,51 @@ def build_arguments(
     return args
 
 
+def build_format_arguments(
+    document_path: str,
+    settings: PluginSettings,
+    extra_arguments: Optional[List[str]] = None,
+) -> List[str]:
+    """
+    Build arguments for ruff format.
+
+    Parameters
+    ----------
+    document : pylsp.workspace.Document
+        Document to apply ruff on.
+    settings : PluginSettings
+        Settings to use for arguments to pass to ruff.
+    extra_arguments : List[str]
+        Extra arguments to pass to ruff.
+
+    Returns
+    -------
+    List containing the arguments.
+    """
+    args = []
+    # Suppress update announcements
+    args.append("--quiet")
+
+    # Always force excludes
+    args.append("--force-exclude")
+    # Pass filename to ruff for per-file-ignores, catch unsaved
+    if document_path != "":
+        args.append(f"--stdin-filename={document_path}")
+
+    if settings.config:
+        args.append(f"--config={settings.config}")
+
+    if settings.exclude:
+        args.append(f"--exclude={','.join(settings.exclude)}")
+
+    if extra_arguments:
+        args.extend(extra_arguments)
+
+    args.extend(["--", "-"])
+
+    return args
+
+
 def load_settings(workspace: Workspace, document_path: str) -> PluginSettings:
     """
     Load settings from pyproject.toml file in the project path.
diff --git a/tests/test_code_actions.py b/tests/test_code_actions.py
@@ -150,37 +150,3 @@ def f():
     settings = ruff_lint.load_settings(workspace, doc.path)
     fixed_str = ruff_lint.run_ruff_fix(doc, settings)
     assert fixed_str == expected_str_safe
-
-
-def test_format_document_default_settings(workspace):
-    _, doc = temp_document(import_str, workspace)
-    settings = ruff_lint.load_settings(workspace, doc.path)
-    formatted_str = ruff_lint.run_ruff_format(
-        settings, document_path=doc.path, document_source=doc.source
-    )
-    assert formatted_str == import_str
-
-
-def test_format_document_settings(workspace):
-    expected_str = dedent(
-        """
-        import os
-        import pathlib
-        """
-    )
-    workspace._config.update(
-        {
-            "plugins": {
-                "ruff": {
-                    "select": ["I"],
-                    "format": ["I001"],
-                }
-            }
-        }
-    )
-    _, doc = temp_document(import_str, workspace)
-    settings = ruff_lint.load_settings(workspace, doc.path)
-    formatted_str = ruff_lint.run_ruff_format(
-        settings, document_path=doc.path, document_source=doc.source
-    )
-    assert formatted_str == expected_str
diff --git a/tests/test_ruff_format.py b/tests/test_ruff_format.py
@@ -0,0 +1,114 @@
+import contextlib
+import tempfile
+import textwrap as tw
+from typing import Any, List, Mapping, Optional
+from unittest.mock import Mock
+
+import pytest
+from pylsp import uris
+from pylsp.config.config import Config
+from pylsp.workspace import Document, Workspace
+
+import pylsp_ruff.plugin as plugin
+
+_UNSORTED_IMPORTS = tw.dedent(
+    """
+    from thirdparty import x
+    import io
+    import asyncio
+    """
+).strip()
+
+_SORTED_IMPORTS = tw.dedent(
+    """
+    import asyncio
+    import io
+
+    from thirdparty import x
+    """
+).strip()
+
+_UNFORMATTED_CODE = tw.dedent(
+    """
+    def foo(): pass
+    def bar(): pass
+    """
+).strip()
+
+_FORMATTED_CODE = tw.dedent(
+    """
+    def foo():
+        pass
+
+
+    def bar():
+        pass
+    """
+).strip()
+
+
+@pytest.fixture()
+def workspace(tmp_path):
+    """Return a workspace."""
+    ws = Workspace(tmp_path.absolute().as_uri(), Mock())
+    ws._config = Config(ws.root_uri, {}, 0, {})
+    return ws
+
+
+def temp_document(doc_text, workspace):
+    with tempfile.NamedTemporaryFile(
+        mode="w", dir=workspace.root_path, delete=False
+    ) as temp_file:
+        name = temp_file.name
+        temp_file.write(doc_text)
+    doc = Document(uris.from_fs_path(name), workspace)
+    return name, doc
+
+
+def run_plugin_format(workspace: Workspace, doc: Document) -> str:
+    class TestResult:
+        result: Optional[List[Mapping[str, Any]]]
+
+        def __init__(self):
+            self.result = None
+
+        def get_result(self):
+            return self.result
+
+        def force_result(self, r):
+            self.result = r
+
+    generator = plugin.pylsp_format_document(workspace, doc)
+    result = TestResult()
+    with contextlib.suppress(StopIteration):
+        generator.send(None)
+        generator.send(result)
+
+    if result.result:
+        return result.result[0]["newText"]
+    return pytest.fail()
+
+
+def test_ruff_format_only(workspace):
+    txt = f"{_UNSORTED_IMPORTS}\n{_UNFORMATTED_CODE}"
+    want = f"{_UNSORTED_IMPORTS}\n\n\n{_FORMATTED_CODE}\n"
+    _, doc = temp_document(txt, workspace)
+    got = run_plugin_format(workspace, doc)
+    assert want == got
+
+
+def test_ruff_format_and_sort_imports(workspace):
+    txt = f"{_UNSORTED_IMPORTS}\n{_UNFORMATTED_CODE}"
+    want = f"{_SORTED_IMPORTS}\n\n\n{_FORMATTED_CODE}\n"
+    _, doc = temp_document(txt, workspace)
+    workspace._config.update(
+        {
+            "plugins": {
+                "ruff": {
+                    "format": ["I001"],
+                }
+            }
+        }
+    )
+    got = run_plugin_format(workspace, doc)
+    assert want == got
diff --git a/tests/test_ruff_lint.py b/tests/test_ruff_lint.py
@@ -178,6 +178,7 @@ def f():
         str(sys.executable),
         "-m",
         "ruff",
+        "check",
         "--quiet",
         "--exit-zero",
         "--output-format=json",
