nREPL server should not send color ANSI esc seqs to the clients (#1040)

Hi,

could you please review patch to prevent the nREPL server sending color
ANSI esc seqs to the clients. It fixes #1039.

I modified the path from the nREPL server down to the Pygments call to
accept a `disable_color` parameter, which stops color formatting.

I've also updated the tests to reflect this change. The
`src/basilisp/lang/source.py` test is currently placed under
`tests/basilisp/lang`, though I noticed the other tests in the
`src/basilisp/` subdirectories don't follow this structure -- not sure
why that is. I'm happy to relocate it to `tests/basilisp` if needed.

Thanks

---------

Co-authored-by: ikappaki <[email protected]>

Author: ikappaki

CHANGELOG.md

@@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 ### Changed
  * The compiler will no longer require `Var` indirection for top-level `do` forms unless those forms specify `^:use-var-indirection` metadata (which currently is only used in the `ns` macro) (#1034)
+ * nREPL server no longer sends ANSI color escape sequences in exception messages to clients (#1039)
 
 ### Fixed
  * Fix a bug where the compiler would always generate inline function definitions even if the `inline-functions` compiler option is disabled (#1023)

src/basilisp/contrib/nrepl_server.lpy

@@ -147,7 +147,8 @@
           (send-value request send-fn [result {:ns (str *ns*)}]))
         (catch python/Exception e
           (debug :eval-exception e)
-          (let [msg (->> (basilisp.lang.exception/format_exception e (type e) (.-__traceback__ e))
+          (let [msg (->> (basilisp.lang.exception/format_exception e (type e) (.-__traceback__ e)
+                                                                   ** :disable-color true)
                          (str/join ""))]
             (handle-error send-fn (assoc request :ns (str *ns*)) msg)))
         (finally

src/basilisp/lang/compiler/exception.py

@@ -114,8 +114,12 @@ def format_compiler_exception(  # pylint: disable=too-many-branches,unused-argum
     e: CompilerException,
     tp: Optional[Type[Exception]] = None,
     tb: Optional[TracebackType] = None,
+    disable_color: Optional[bool] = None,
 ) -> List[str]:
-    """Format a compiler exception as a list of newline-terminated strings."""
+    """Format a compiler exception as a list of newline-terminated strings.
+
+    If `disable_color` is True, no color formatting will be applied to the source
+    code."""
     context_exc: Optional[BaseException] = e.__cause__
 
     lines = [os.linesep]
@@ -154,7 +158,9 @@ def format_compiler_exception(  # pylint: disable=too-many-branches,unused-argum
     # derive source lines, but use the inner cause exception to place a marker
     # around the error.
     if line is not None and (
-        context_lines := format_source_context(e.filename, line, end_line=end_line)
+        context_lines := format_source_context(
+            e.filename, line, end_line=end_line, disable_color=disable_color
+        )
     ):
         lines.append(f"    context:{os.linesep}")
         lines.append(os.linesep)

src/basilisp/lang/exception.py

@@ -25,17 +25,21 @@ def __str__(self):
 
 
 @functools.singledispatch
-def format_exception(
+def format_exception(  # pylint: disable=unused-argument
     e: Optional[BaseException],
     tp: Optional[Type[BaseException]] = None,
     tb: Optional[TracebackType] = None,
+    disable_color: Optional[bool] = None,
 ) -> List[str]:
     """Format an exception into something readable, returning a list of newline
     terminated strings.
 
     For the majority of Python exceptions, this will just be the result from calling
     `traceback.format_exception`. For Basilisp specific compilation errors, a custom
-    output will be returned."""
+    output will be returned.
+
+    If `disable_color` is True, no color formatting should be applied to the source
+    code."""
     if isinstance(e, BaseException):
         if tp is None:
             tp = type(e)

src/basilisp/lang/reader.py

@@ -166,7 +166,11 @@ def format_syntax_error(  # pylint: disable=unused-argument
     e: SyntaxError,
     tp: Optional[Type[Exception]] = None,
     tb: Optional[TracebackType] = None,
+    disable_color: Optional[bool] = None,
 ) -> List[str]:
+    """If `disable_color` is True, no color formatting will be applied to the source
+    code."""
+
     context_exc: Optional[BaseException] = e.__cause__
 
     lines = [os.linesep]
@@ -199,7 +203,11 @@ def format_syntax_error(  # pylint: disable=unused-argument
     if (
         e.filename is not None
         and e.line is not None
-        and (context_lines := format_source_context(e.filename, e.line))
+        and (
+            context_lines := format_source_context(
+                e.filename, e.line, disable_color=disable_color
+            )
+        )
     ):
         lines.append(f"    context:{os.linesep}")
         lines.append(os.linesep)

src/basilisp/lang/source.py

@@ -9,17 +9,24 @@
     import pygments.styles
 except ImportError:  # pragma: no cover
 
-    def _format_source(s: str) -> str:
+    def _format_source(
+        s: str, disable_color: Optional[bool] = None  # pylint: disable=unused-argument
+    ) -> str:
         return f"{s}{os.linesep}"
 
 else:
 
-    def _get_formatter_name() -> Optional[str]:  # pragma: no cover
+    def _get_formatter_name(
+        disable_color: Optional[bool] = None,
+    ) -> Optional[str]:  # pragma: no cover
         """Get the Pygments formatter name for formatting the source code by
         inspecting various environment variables set by terminals.
 
-        If `BASILISP_NO_COLOR` is set to a truthy value, use no formatting."""
-        if os.environ.get("BASILISP_NO_COLOR", "false").lower() in {"1", "true"}:
+        If If `disable_color` is explicitly True or `BASILISP_NO_COLOR` is set
+        to a truthy value, use no formatting."""
+        if (disable_color is True) or os.environ.get(
+            "BASILISP_NO_COLOR", "false"
+        ).lower() in {"1", "true"}:
             return None
         elif os.environ.get("COLORTERM", "") in {"truecolor", "24bit"}:
             return "terminal16m"
@@ -28,9 +35,13 @@ def _get_formatter_name() -> Optional[str]:  # pragma: no cover
         else:
             return "terminal"
 
-    def _format_source(s: str) -> str:  # pragma: no cover
-        """Format source code for terminal output."""
-        if (formatter_name := _get_formatter_name()) is None:
+    def _format_source(
+        s: str, disable_color: Optional[bool] = None
+    ) -> str:  # pragma: no cover
+        """Format source code for terminal output.
+
+        If `disable_color` is True, no formatting will be applied to the source code."""
+        if (formatter_name := _get_formatter_name(disable_color)) is None:
             return f"{s}{os.linesep}"
         return pygments.highlight(
             s,
@@ -41,15 +52,19 @@ def _format_source(s: str) -> str:  # pragma: no cover
         )
 
 
-def format_source_context(  # pylint: disable=too-many-locals
+def format_source_context(  # pylint: disable=too-many-arguments,too-many-locals
     filename: str,
     line: int,
     end_line: Optional[int] = None,
     num_context_lines: int = 5,
     show_cause_marker: bool = True,
+    disable_color: Optional[bool] = None,
 ) -> List[str]:
     """Format source code context with line numbers and identifiers for the affected
-    line(s)."""
+    line(s).
+
+    If `disable_color` is True, no color formatting will be applied to the source code.
+    """
     assert num_context_lines >= 0
 
     lines = []
@@ -87,7 +102,7 @@ def format_source_context(  # pylint: disable=too-many-locals
 
                 line_num = str(n + 1).rjust(num_justify)
                 lines.append(
-                    f"{line_num}{line_marker}| {_format_source(source_line.rstrip())}"
+                    f"{line_num}{line_marker}| {_format_source(source_line.rstrip(), disable_color=disable_color)}"
                 )
 
     return lines

tests/basilisp/compiler_test.py

@@ -220,13 +220,12 @@ def test_shows_source_context(
                 ).lstrip()
                 + ("\n" if include_newline else "")
             )
-            monkeypatch.setenv("BASILISP_NO_COLOR", "true")
             monkeypatch.syspath_prepend(source_file.parent)
 
             with pytest.raises(compiler.CompilerException) as e:
                 lcompile("(require 'compiler-test)")
 
-            formatted = format_exception(e.value)
+            formatted = format_exception(e.value, disable_color=True)
             assert re.match(
                 (
                     rf"{os.linesep}"

tests/basilisp/contrib/nrepl_server_test.lpy

@@ -596,10 +596,7 @@
                     " 7   |   (xyz))"
                     " 8   | "
                     " 9   | (afn)"]
-                   (-> err
-                       ;; remove ansi control chars
-                       (str/replace #"\\x1B[@-_][0-?]*[ -/]*[@-~]" "")
-                       (str/split-lines)))))
+                   (str/split-lines err))))
           (let [{:keys [id ex status ns]} (client-recv! client)]
             (is (= @id* id))
             (is (= "abc.xyz" ns))

tests/basilisp/reader_test.py

@@ -254,13 +254,12 @@ def test_shows_source_context(self, monkeypatch, source_file: Path):
                     """
                 ).strip()
             )
-            monkeypatch.setenv("BASILISP_NO_COLOR", "true")
             monkeypatch.syspath_prepend(source_file.parent)
 
             with pytest.raises(reader.SyntaxError) as e:
                 list(reader.read_file(source_file))
 
-            v = format_exception(e.value)
+            v = format_exception(e.value, disable_color=True)
             assert re.match(
                 (
                     rf"{os.linesep}"

tests/basilisp/source_test.py

@@ -0,0 +1,64 @@
+import os
+import textwrap
+from pathlib import Path
+
+import pytest
+
+from basilisp.lang import compiler as compiler
+from basilisp.lang.source import format_source_context
+
+
+@pytest.fixture
+def source_file(tmp_path: Path) -> Path:
+    return tmp_path / "source_test.lpy"
+
+
+@pytest.fixture
+def source_file_path(source_file: Path) -> str:
+    return str(source_file)
+
+
+def test_format_source_context(monkeypatch, source_file, source_file_path):
+    source_file.write_text(
+        textwrap.dedent(
+            """
+            (ns source-test)
+            
+            (a)
+            (let [a 5]
+              (b))
+            """
+        )
+    )
+    format_c = format_source_context(source_file_path, 2, end_line=4)
+    assert [
+        " 1   | \x1b[37m\x1b[39;49;00m\n",
+        " 2 > | (\x1b[34mns \x1b[39;49;00m\x1b[31msource-test\x1b[39;49;00m)\x1b[37m\x1b[39;49;00m\n",
+        " 3 > | \x1b[37m\x1b[39;49;00m\n",
+        " 4 > | (\x1b[32ma\x1b[39;49;00m)\x1b[37m\x1b[39;49;00m\n",
+        " 5   | (\x1b[34mlet \x1b[39;49;00m[\x1b[31ma\x1b[39;49;00m\x1b[37m \x1b[39;49;00m\x1b[34m5\x1b[39;49;00m]\x1b[37m\x1b[39;49;00m\n",
+        " 6   | \x1b[37m  \x1b[39;49;00m(\x1b[32mb\x1b[39;49;00m))\x1b[37m\x1b[39;49;00m\n",
+    ] == format_c
+
+    format_nc = format_source_context(
+        source_file_path, 2, end_line=4, disable_color=True
+    )
+    assert [
+        " 1   | " + os.linesep,
+        " 2 > | (ns source-test)" + os.linesep,
+        " 3 > | " + os.linesep,
+        " 4 > | (a)" + os.linesep,
+        " 5   | (let [a 5]" + os.linesep,
+        " 6   |   (b))" + os.linesep,
+    ] == format_nc
+
+    monkeypatch.setenv("BASILISP_NO_COLOR", "true")
+    format_bnc = format_source_context(source_file_path, 2, end_line=4)
+    assert [
+        " 1   | " + os.linesep,
+        " 2 > | (ns source-test)" + os.linesep,
+        " 3 > | " + os.linesep,
+        " 4 > | (a)" + os.linesep,
+        " 5   | (let [a 5]" + os.linesep,
+        " 6   |   (b))" + os.linesep,
+    ] == format_bnc