Merge pull request #160 from source-foundry/color-as-default

Convert to ANSI escape code colored diff output as default in terminals

Author: chrissimpkins

lib/fdiff/__main__.py

@@ -37,14 +37,7 @@ def run(argv: List[Text]) -> None:
     parser = argparse.ArgumentParser(description="An OpenType table diff tool for fonts.")
     parser.add_argument("--version", action="version", version=f"fdiff v{__version__}")
     parser.add_argument(
-        "-c",
-        "--color",
-        action="store_true",
-        default=False,
-        help="ANSI escape code colored diff",
-    )
-    parser.add_argument(
-        "-l", "--lines", type=int, default=3, help="Number of context lines (default 3)"
+        "-l", "--lines", type=int, default=3, help="Number of context lines (default: 3)"
     )
     parser.add_argument(
         "--include",
@@ -60,10 +53,25 @@ def run(argv: List[Text]) -> None:
     )
     parser.add_argument("--head", type=int, help="Display first n lines of output")
     parser.add_argument("--tail", type=int, help="Display last n lines of output")
+    parser.add_argument("--external", type=str, help="Run external diff tool command")
     parser.add_argument(
-        "--nomp", action="store_true", help="Do not use multi process optimizations"
+        "-c",
+        "--color",
+        action="store_true",
+        default=False,
+        help="Force ANSI escape code color formatting in all environments",
+    )
+    parser.add_argument(
+        "--nomp",
+        action="store_true",
+        help="Do not use multi process optimizations (default: on)",
+    )
+    parser.add_argument(
+        "--nocolor",
+        action="store_true",
+        default=False,
+        help="Do not use ANSI escape code colored diff (default: on)",
     )
-    parser.add_argument("--external", type=str, help="Run external diff tool command")
     parser.add_argument("PREFILE", help="Font file path/URL 1")
     parser.add_argument("POSTFILE", help="Font file path/URL 2")
 
@@ -154,8 +162,12 @@ def run(argv: List[Text]) -> None:
 
                 # write stdout from external tool
                 for line, exit_code in ext_diff:
-                    # format with color if color flag is entered on command line
-                    if args.color:
+                    # format with color by default unless:
+                    #   (1) user entered the --nocolor option
+                    #   (2) we are not piping std output to a terminal
+                    # Force formatting with color in all environments if the user includes
+                    # the `-c` / `--color` option
+                    if (not args.nocolor and console.is_terminal) or args.color:
                         sys.stdout.write(color_unified_diff_line(line))
                     else:
                         sys.stdout.write(line)
@@ -194,7 +206,12 @@ def run(argv: List[Text]) -> None:
 
             # print unified diff results to standard output stream
             has_diff = False
-            if args.color:
+            # format with color by default unless:
+            #   (1) user entered the --nocolor option
+            #   (2) we are not piping std output to a terminal
+            # Force formatting with color in all environments if the user includes
+            # the `-c` / `--color` option
+            if (not args.nocolor and console.is_terminal) or args.color:
                 for line in iterable:
                     has_diff = True
                     sys.stdout.write(color_unified_diff_line(line))
@@ -203,7 +220,7 @@ def run(argv: List[Text]) -> None:
                     has_diff = True
                     sys.stdout.write(line)
 
-        # if no difference was found, tell the user instead of
-        # simply closing with zero exit status code.
+        # if no difference was found, tell the user in addition to the
+        # the zero exit status code.
         if not has_diff:
-            print("[*] There is no difference between the files.")
+            print("[*] There is no difference in the tested OpenType tables.")

tests/test_main.py

@@ -2,19 +2,33 @@
 # -*- coding: utf-8 -*-
 
 import os
+import sys
+from unittest.mock import MagicMock
 
 import pytest
 
 from fdiff.__main__ import run
 
 ROBOTO_BEFORE_PATH = os.path.join("tests", "testfiles", "Roboto-Regular.subset1.ttf")
 ROBOTO_AFTER_PATH = os.path.join("tests", "testfiles", "Roboto-Regular.subset2.ttf")
-ROBOTO_UDIFF_EXPECTED_PATH = os.path.join("tests", "testfiles", "roboto_udiff_expected.txt")
-ROBOTO_UDIFF_COLOR_EXPECTED_PATH = os.path.join("tests", "testfiles", "roboto_udiff_color_expected.txt")
-ROBOTO_UDIFF_1CONTEXT_EXPECTED_PATH = os.path.join("tests", "testfiles", "roboto_udiff_1context_expected.txt")
-ROBOTO_UDIFF_HEADONLY_EXPECTED_PATH = os.path.join("tests", "testfiles", "roboto_udiff_headonly_expected.txt")
-ROBOTO_UDIFF_HEADPOSTONLY_EXPECTED_PATH = os.path.join("tests", "testfiles", "roboto_udiff_headpostonly_expected.txt")
-ROBOTO_UDIFF_EXCLUDE_HEADPOST_EXPECTED_PATH = os.path.join("tests", "testfiles", "roboto_udiff_ex_headpost_expected.txt")
+ROBOTO_UDIFF_EXPECTED_PATH = os.path.join(
+    "tests", "testfiles", "roboto_udiff_expected.txt"
+)
+ROBOTO_UDIFF_COLOR_EXPECTED_PATH = os.path.join(
+    "tests", "testfiles", "roboto_udiff_color_expected.txt"
+)
+ROBOTO_UDIFF_1CONTEXT_EXPECTED_PATH = os.path.join(
+    "tests", "testfiles", "roboto_udiff_1context_expected.txt"
+)
+ROBOTO_UDIFF_HEADONLY_EXPECTED_PATH = os.path.join(
+    "tests", "testfiles", "roboto_udiff_headonly_expected.txt"
+)
+ROBOTO_UDIFF_HEADPOSTONLY_EXPECTED_PATH = os.path.join(
+    "tests", "testfiles", "roboto_udiff_headpostonly_expected.txt"
+)
+ROBOTO_UDIFF_EXCLUDE_HEADPOST_EXPECTED_PATH = os.path.join(
+    "tests", "testfiles", "roboto_udiff_ex_headpost_expected.txt"
+)
 
 ROBOTO_BEFORE_URL = "https://github.com/source-foundry/fdiff/raw/master/tests/testfiles/Roboto-Regular.subset1.ttf"
 ROBOTO_AFTER_URL = "https://github.com/source-foundry/fdiff/raw/master/tests/testfiles/Roboto-Regular.subset2.ttf"
@@ -80,28 +94,41 @@ def test_main_filepath_validations_false_secondfont(capsys):
 #  Mutually exclusive argument tests
 #
 
+
 def test_main_include_exclude_defined_simultaneously(capsys):
-    args = ["--include", "head", "--exclude", "head", ROBOTO_BEFORE_PATH, ROBOTO_AFTER_PATH]
+    args = [
+        "--include",
+        "head",
+        "--exclude",
+        "head",
+        ROBOTO_BEFORE_PATH,
+        ROBOTO_AFTER_PATH,
+    ]
 
     with pytest.raises(SystemExit) as exit_info:
         run(args)
 
     captured = capsys.readouterr()
-    assert captured.err.startswith("[*] Error: --include and --exclude are mutually exclusive options")
+    assert captured.err.startswith(
+        "[*] Error: --include and --exclude are mutually exclusive options"
+    )
     assert exit_info.value.code == 1
 
 
 #
 #  Unified diff integration tests
 #
 
+
 def test_main_run_unified_default_local_files_no_diff(capsys):
     """Test default behavior when there is no difference in font files under evaluation"""
     args = [ROBOTO_BEFORE_PATH, ROBOTO_BEFORE_PATH]
 
     run(args)
     captured = capsys.readouterr()
-    assert captured.out.startswith("[*] There is no difference between the files.")
+    assert captured.out.startswith(
+        "[*] There is no difference in the tested OpenType tables"
+    )
 
 
 def test_main_run_unified_default_local_files(capsys):
@@ -180,24 +207,24 @@ def test_main_run_unified_default_404(capsys):
 
 
 def test_main_run_unified_color(capsys):
-    args = ["-c", ROBOTO_BEFORE_PATH, ROBOTO_AFTER_PATH]
+    # prior to v3.0.0, the `-c` / `--color` option was required for color output
+    # this is the default as of v3.0.0 and the test arguments were
+    # modified here
+    args = [ROBOTO_BEFORE_PATH, ROBOTO_AFTER_PATH]
+    # we also need to mock sys.stdout.isatty because color does not
+    # show when this returns False
+    sys.stdout.isatty = MagicMock(return_value=True)
 
     run(args)
     captured = capsys.readouterr()
-
+    # spot checks for escape code start sequence
     res_string_list = captured.out.split("\n")
-    expected_string_list = ROBOTO_UDIFF_COLOR_EXPECTED.split("\n")
-
-    # have to handle the tests for the top two file path lines
-    # differently than the rest of the comparisons because
-    # the time is defined using local platform settings
-    # which makes tests fail on different remote CI testing services
-    for x, line in enumerate(res_string_list):
-        # treat top two lines of the diff as comparison of first 10 chars only
-        if x in (0, 1):
-            assert line[0:9] == expected_string_list[x][0:9]
-        else:
-            assert line == expected_string_list[x]
+    assert captured.out.startswith("\x1b")
+    assert res_string_list[10].startswith("\x1b")
+    assert res_string_list[71].startswith("\x1b")
+    assert res_string_list[180].startswith("\x1b")
+    assert res_string_list[200].startswith("\x1b")
+    assert res_string_list[238].startswith("\x1b")
 
 
 def test_main_run_unified_context_lines_1(capsys):

tests/test_main_unix_only.py

@@ -2,15 +2,20 @@
 
 import os
 import sys
+from unittest.mock import MagicMock
 
 import pytest
 
 from fdiff.__main__ import run
 
 ROBOTO_BEFORE_PATH = os.path.join("tests", "testfiles", "Roboto-Regular.subset1.ttf")
 ROBOTO_AFTER_PATH = os.path.join("tests", "testfiles", "Roboto-Regular.subset2.ttf")
-ROBOTO_EXTDIFF_EXPECTED_PATH = os.path.join("tests", "testfiles", "roboto_extdiff_expected.txt")
-ROBOTO_EXTDIFF_COLOR_EXPECTED_PATH = os.path.join("tests", "testfiles", "roboto_extdiff_color_expected.txt")
+ROBOTO_EXTDIFF_EXPECTED_PATH = os.path.join(
+    "tests", "testfiles", "roboto_extdiff_expected.txt"
+)
+ROBOTO_EXTDIFF_COLOR_EXPECTED_PATH = os.path.join(
+    "tests", "testfiles", "roboto_extdiff_color_expected.txt"
+)
 
 ROBOTO_BEFORE_URL = "https://github.com/source-foundry/fdiff/raw/master/tests/testfiles/Roboto-Regular.subset1.ttf"
 ROBOTO_AFTER_URL = "https://github.com/source-foundry/fdiff/raw/master/tests/testfiles/Roboto-Regular.subset2.ttf"
@@ -72,23 +77,28 @@ def test_main_external_diff_remote(capsys):
 
 
 def test_main_external_diff_color(capsys):
-    args = ["--external", "diff -u", "--color", ROBOTO_BEFORE_PATH, ROBOTO_AFTER_PATH]
-    expected_string_list = ROBOTO_EXTDIFF_COLOR_EXPECTED.split("\n")
+    # prior to v3.0.0, the `-c` / `--color` option was required for color output
+    # this is the default as of v3.0.0 and the test arguments were
+    # modified here
+    args = ["--external", "diff -u", ROBOTO_BEFORE_PATH, ROBOTO_AFTER_PATH]
+    # we also need to patch sys.stdout.isatty because color does not
+    # show when this returns False
+    sys.stdout.isatty = MagicMock(return_value=True)
+    # expected_string_list = ROBOTO_EXTDIFF_COLOR_EXPECTED.split("\n")
 
     with pytest.raises(SystemExit):
         run(args)
 
     captured = capsys.readouterr()
+
+    # spot checks for escape code start sequence
     res_string_list = captured.out.split("\n")
-    for x, line in enumerate(res_string_list):
-        # treat top two lines of the diff as comparison of first 3 chars only
-        if x in (0, 1):
-            assert line[0:2] == expected_string_list[x][0:2]
-        elif x in range(2, 10):
-            assert line == expected_string_list[x]
-        else:
-            # skip lines beyond first 10
-            pass
+    assert captured.out.startswith("\x1b")
+    assert res_string_list[10].startswith("\x1b")
+    assert res_string_list[71].startswith("\x1b")
+    assert res_string_list[180].startswith("\x1b")
+    assert res_string_list[200].startswith("\x1b")
+    assert res_string_list[238].startswith("\x1b")
 
 
 def test_main_external_diff_with_head_fails(capsys):
@@ -114,11 +124,18 @@ def test_main_external_diff_with_tail_fails(capsys):
 
 
 def test_main_external_diff_with_lines_fails(capsys):
-    args = ["--external", "diff -u", "--lines", "1", ROBOTO_BEFORE_PATH, ROBOTO_AFTER_PATH]
+    args = [
+        "--external",
+        "diff -u",
+        "--lines",
+        "1",
+        ROBOTO_BEFORE_PATH,
+        ROBOTO_AFTER_PATH,
+    ]
 
     with pytest.raises(SystemExit) as exit_info:
         run(args)
 
     captured = capsys.readouterr()
     assert "[ERROR] The lines option is not supported" in captured.err
-    assert exit_info.value.code == 1
+    assert exit_info.value.code == 1

tox.ini

@@ -1,5 +1,5 @@
 [tox]
-envlist = py39
+envlist = py310
 
 [testenv]
 commands =