diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index efb135f6..059040b2 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -32,6 +32,8 @@ repos: rev: v2.4.2 hooks: - id: codespell + # iTerm is a terminal emulator, named in the hyperlink support probe. + args: [--ignore-words-list=iterm] - repo: https://github.com/commit-check/commit-check rev: v2.11.0 hooks: diff --git a/commit_check/rule_builder.py b/commit_check/rule_builder.py index 499c9933..762fd39a 100644 --- a/commit_check/rule_builder.py +++ b/commit_check/rule_builder.py @@ -234,16 +234,24 @@ def _build_length_rule( if not isinstance(length, int): return None + # The suggestion is templated on the same values as the error: naming + # the configured length is the whole point of the advice, and "the + # configured minimum" told the reader less than the error above it. error = ( catalog_entry.error.format(max_len=length, min_len=length) if catalog_entry.error else None ) + suggest = ( + catalog_entry.suggest.format(max_len=length, min_len=length) + if catalog_entry.suggest + else None + ) return ValidationRule( check=catalog_entry.check, error=error, - suggest=catalog_entry.suggest, + suggest=suggest, value=length, ) diff --git a/commit_check/rules_catalog.py b/commit_check/rules_catalog.py index 7651a18f..29717b2e 100644 --- a/commit_check/rules_catalog.py +++ b/commit_check/rules_catalog.py @@ -73,14 +73,14 @@ def docs_url(self) -> str | None: check="subject_max_length", regex=None, error="Subject must be at most {max_len} characters", - suggest="Keep the subject concise (<= configured max)", + suggest="Shorten the subject to {max_len} characters or fewer", ), RuleCatalogEntry( rule_id="CC005", check="subject_min_length", regex=None, error="Subject must be at least {min_len} characters", - suggest="Provide a meaningful subject (>= configured min)", + suggest="Write a subject of at least {min_len} characters", ), RuleCatalogEntry( rule_id="CC006", diff --git a/commit_check/util.py b/commit_check/util.py index ee62f270..8053d8c3 100644 --- a/commit_check/util.py +++ b/commit_check/util.py @@ -8,6 +8,7 @@ from __future__ import annotations import os import subprocess +import sys from subprocess import CalledProcessError from commit_check import RED, GREEN, YELLOW, RESET_COLOR @@ -27,17 +28,24 @@ def _print_failure( return if not no_banner and not print_error_header.has_been_called: print_error_header() + docs_url = check.get("docs_url", "") or "" print_error_message( check["check"], check.get("error", ""), actual, rule_id=rule_id, + docs_url=docs_url, ) if check.get("suggest"): print_suggestion(check["suggest"]) - docs_url = check.get("docs_url", "") - if docs_url: + # When the ID above is already a link, repeating the URL only adds a line + # to read. Without hyperlink support — a pipe, a CI log — it is the only + # way the reader gets the address at all, so it stays. + if docs_url and not (rule_id and supports_hyperlinks()): print(f"Docs: {docs_url}") + # Blank line closes the whole block, rather than splitting it before the + # documentation link. + print() def get_branch_name() -> str: @@ -294,22 +302,78 @@ def print_error_header(): print(" ") -def print_error_message(check_type: str, error: str, reason: str, rule_id: str = ""): +#: Terminals known to render OSC 8 hyperlinks, by ``TERM_PROGRAM``. +_HYPERLINK_TERM_PROGRAMS = frozenset( + {"iTerm.app", "WezTerm", "vscode", "Hyper", "ghostty", "rio"} +) + + +def supports_hyperlinks() -> bool: + """Whether the terminal renders OSC 8 hyperlinks. + + A terminal that does not understand the escape sequence may print its + payload as visible junk, so this errs towards saying no. The signals are + the ones the wider tooling ecosystem settled on, which is why a link that + works in ``ruff`` works here too. + + Piped or redirected output always says no: the sequence would end up in + the file, and a CI log is read as plain text. + + ``FORCE_HYPERLINK`` overrides the detection in both directions, following + the convention ``FORCE_COLOR`` established: ``0`` turns links off even on a + terminal that renders them, any other value turns them on. + """ + forced = os.environ.get("FORCE_HYPERLINK") + if forced: + return forced != "0" + if not sys.stdout.isatty(): + return False + if os.environ.get("TERM") == "dumb": + return False + if os.environ.get("TERM_PROGRAM") in _HYPERLINK_TERM_PROGRAMS: + return True + if "kitty" in os.environ.get("TERM", ""): + return True + # GNOME Terminal and the other VTE-based terminals, from 0.50 onwards. + try: + return int(os.environ.get("VTE_VERSION", "0")) >= 5000 + except ValueError: + return False + + +def hyperlink(text: str, url: str) -> str: + """Wrap ``text`` in an OSC 8 hyperlink pointing at ``url``.""" + return f"\033]8;;{url}\033\\{text}\033]8;;\033\\" + + +def print_error_message( + check_type: str, + error: str, + reason: str, + rule_id: str = "", + docs_url: str = "", +) -> None: """Print error message. :param check_type: the check that failed, e.g. ``subject_imperative`` :param error: the human-readable explanation of the failure :param reason: the offending value :param rule_id: stable rule ID, e.g. ``CC003`` (omitted when empty) + :param docs_url: the rule's documentation, linked from the ID when the + terminal supports it :returns: Give error messages to user """ - prefix = f"{YELLOW}{rule_id}{RESET_COLOR} " if rule_id else "" + # The kebab-case form is what the rules reference uses as its headings, so + # the name printed here can be searched for there verbatim. + name = check_type.replace("_", "-") + label = rule_id + if rule_id and docs_url and supports_hyperlinks(): + label = hyperlink(rule_id, docs_url) + prefix = f"{YELLOW}{label}{RESET_COLOR} " if rule_id else "" print( - f"{prefix}{YELLOW}{check_type}{RESET_COLOR} check failed ==> {RED}{reason}{RESET_COLOR} ", - end="", + f"{prefix}{YELLOW}{name}{RESET_COLOR} check failed ==> {RED}{reason}{RESET_COLOR}" ) - print("") if error: print(error) @@ -319,8 +383,4 @@ def print_suggestion(suggest: str) -> None: :param suggest: what message to print out """ if suggest: - print( - f"Suggest: {GREEN}{suggest}{RESET_COLOR} ", - end="", - ) - print("\n") + print(f"Suggest: {GREEN}{suggest}{RESET_COLOR}") diff --git a/tests/rule_builder_test.py b/tests/rule_builder_test.py index 8f50a036..7a96fb0e 100644 --- a/tests/rule_builder_test.py +++ b/tests/rule_builder_test.py @@ -438,3 +438,37 @@ def test_build_all_rules_no_ai_by_default(self): rules = builder.build_all_rules() ai_rules = [r for r in rules if r.check.startswith("ai_")] assert len(ai_rules) == 0 + + +class TestLengthRuleMessages: + """The configured limit reaches both the error and the advice. + + The suggestion used to name no length at all — "Provide a meaningful + subject (>= configured min)" sat directly under an error that had already + said "at least 5 characters", so the advice was vaguer than the complaint + above it. + """ + + @pytest.mark.benchmark + def test_min_length_names_the_limit(self): + rules = RuleBuilder({"commit": {"subject_min_length": 12}}).build_all_rules() + rule = next(r for r in rules if r.check == "subject_min_length") + assert rule.error == "Subject must be at least 12 characters" + assert rule.suggest == "Write a subject of at least 12 characters" + + @pytest.mark.benchmark + def test_max_length_names_the_limit(self): + rules = RuleBuilder({"commit": {"subject_max_length": 72}}).build_all_rules() + rule = next(r for r in rules if r.check == "subject_max_length") + assert rule.error == "Subject must be at most 72 characters" + assert rule.suggest == "Shorten the subject to 72 characters or fewer" + + @pytest.mark.benchmark + @pytest.mark.parametrize("check", ["subject_min_length", "subject_max_length"]) + def test_no_placeholder_survives_into_output(self, check): + """A missed substitution would print a literal brace to the user.""" + rules = RuleBuilder({"commit": {check: 40}}).build_all_rules() + rule = next(r for r in rules if r.check == check) + assert "{" not in (rule.error or "") + assert "{" not in (rule.suggest or "") + assert "40" in (rule.suggest or "") diff --git a/tests/util_test.py b/tests/util_test.py index 33651195..e325198b 100644 --- a/tests/util_test.py +++ b/tests/util_test.py @@ -15,6 +15,9 @@ print_error_header, print_error_message, print_suggestion, + supports_hyperlinks, + hyperlink, + _print_failure, ) from subprocess import CalledProcessError, PIPE from unittest.mock import MagicMock @@ -577,26 +580,179 @@ def test_print_error_header(self, capfd): @pytest.mark.benchmark @pytest.mark.parametrize( - "check_type, type_failed_msg", + "check_type, printed_name", [ - ("message", "check failed ==>"), - ("branch", "check failed ==>"), - ("author_name", "check failed ==>"), - ("author_email", "check failed ==>"), - ("signoff", "check failed ==>"), + ("message", "message"), + ("branch", "branch"), + # The config key is snake_case, but the rules reference titles + # its sections in kebab-case. The output follows the reference, + # so a name read here can be searched for there verbatim. + ("author_name", "author-name"), + ("author_email", "author-email"), + ("signoff", "signoff"), ], ) @pytest.mark.benchmark - def test_print_error_message(self, capfd, check_type, type_failed_msg): + def test_print_error_message(self, capfd, check_type, printed_name): # Must print on stdout with given argument. dummy_reason = "failure reason" dummy_error = "dummy error" print_error_message(check_type, dummy_error, dummy_reason) stdout, _ = capfd.readouterr() - assert check_type in stdout - assert type_failed_msg in stdout + assert printed_name in stdout + assert "_" not in stdout.split(" check failed")[0] + assert "check failed ==>" in stdout assert dummy_error in stdout + class TestHyperlinks: + """The rule ID doubles as a link to its documentation. + + Only where that renders: a terminal that does not understand OSC 8 may + print the escape payload as visible junk, and in a CI log the sequence + is noise around a URL the reader can no longer click anyway. + """ + + @pytest.mark.benchmark + def test_hyperlink_wraps_text_in_osc8(self): + assert hyperlink("CC001", "https://example.com/#cc001") == ( + "\033]8;;https://example.com/#cc001\033\\CC001\033]8;;\033\\" + ) + + @pytest.mark.benchmark + def test_not_supported_when_piped(self, mocker): + mocker.patch.dict("os.environ", {}, clear=True) + mocker.patch("sys.stdout.isatty", return_value=False) + assert supports_hyperlinks() is False + + @pytest.mark.benchmark + def test_forced_even_when_piped(self, mocker): + mocker.patch.dict("os.environ", {"FORCE_HYPERLINK": "1"}, clear=True) + mocker.patch("sys.stdout.isatty", return_value=False) + assert supports_hyperlinks() is True + + @pytest.mark.benchmark + def test_force_zero_turns_links_off(self, mocker): + """Setting it to 0 must not read as "set, therefore on".""" + mocker.patch.dict( + "os.environ", + {"FORCE_HYPERLINK": "0", "TERM_PROGRAM": "WezTerm"}, + clear=True, + ) + mocker.patch("sys.stdout.isatty", return_value=True) + assert supports_hyperlinks() is False + + @pytest.mark.benchmark + def test_force_empty_falls_through_to_detection(self, mocker): + mocker.patch.dict("os.environ", {"FORCE_HYPERLINK": ""}, clear=True) + mocker.patch("sys.stdout.isatty", return_value=False) + assert supports_hyperlinks() is False + + @pytest.mark.benchmark + @pytest.mark.parametrize( + "env, expected", + [ + ({"TERM_PROGRAM": "WezTerm"}, True), + ({"TERM_PROGRAM": "iTerm.app"}, True), + ({"TERM_PROGRAM": "vscode"}, True), + ({"TERM": "xterm-kitty"}, True), + ({"VTE_VERSION": "6003"}, True), + ({"VTE_VERSION": "4000"}, False), + # Malformed rather than absent; must not raise. + ({"VTE_VERSION": "not-a-number"}, False), + ({"TERM": "xterm"}, False), + ({"TERM": "dumb", "TERM_PROGRAM": "WezTerm"}, False), + ], + ) + def test_terminal_detection(self, mocker, env, expected): + mocker.patch.dict("os.environ", env, clear=True) + mocker.patch("sys.stdout.isatty", return_value=True) + assert supports_hyperlinks() is expected + + @pytest.mark.benchmark + def test_id_is_linked_when_supported(self, capfd, mocker): + mocker.patch("commit_check.util.supports_hyperlinks", return_value=True) + print_error_message( + "subject_min_length", + "too short", + "hi", + rule_id="CC005", + docs_url="https://commit-check.com/rules/#cc005", + ) + stdout, _ = capfd.readouterr() + assert "\033]8;;https://commit-check.com/rules/#cc005\033\\" in stdout + + @pytest.mark.benchmark + def test_id_is_plain_when_unsupported(self, capfd, mocker): + mocker.patch("commit_check.util.supports_hyperlinks", return_value=False) + print_error_message( + "subject_min_length", + "too short", + "hi", + rule_id="CC005", + docs_url="https://commit-check.com/rules/#cc005", + ) + stdout, _ = capfd.readouterr() + assert "\033]8;;" not in stdout + assert "CC005" in stdout + + @pytest.mark.benchmark + def test_docs_line_kept_without_hyperlinks(self, capfd, mocker): + """A CI log is where the printed URL is the only way to reach it.""" + mocker.patch("commit_check.util.supports_hyperlinks", return_value=False) + _print_failure( + { + "check": "subject_min_length", + "error": "too short", + "suggest": "write more", + "rule_id": "CC005", + "docs_url": "https://commit-check.com/rules/#cc005", + }, + "hi", + no_banner=True, + ) + stdout, _ = capfd.readouterr() + assert "Docs: https://commit-check.com/rules/#cc005" in stdout + + @pytest.mark.benchmark + def test_docs_line_dropped_when_id_is_the_link(self, capfd, mocker): + """Otherwise every failure spends a line repeating its own link.""" + mocker.patch("commit_check.util.supports_hyperlinks", return_value=True) + _print_failure( + { + "check": "subject_min_length", + "error": "too short", + "suggest": "write more", + "rule_id": "CC005", + "docs_url": "https://commit-check.com/rules/#cc005", + }, + "hi", + no_banner=True, + ) + stdout, _ = capfd.readouterr() + assert "Docs: " not in stdout + assert "\033]8;;" in stdout + + @pytest.mark.benchmark + def test_blank_line_closes_the_block(self, capfd, mocker): + """The separator belongs between rules, not inside one.""" + mocker.patch("commit_check.util.supports_hyperlinks", return_value=False) + _print_failure( + { + "check": "subject_min_length", + "error": "too short", + "suggest": "write more", + "rule_id": "CC005", + "docs_url": "https://commit-check.com/rules/#cc005", + }, + "hi", + no_banner=True, + ) + stdout, _ = capfd.readouterr() + lines = stdout.splitlines() + assert lines[-1] == "" + assert lines[-2].startswith("Docs: ") + assert "" not in lines[:-1] + class TestPrintSuggestion: @pytest.mark.benchmark def test_print_suggestion(self, capfd):