diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 059040b2..b5737f12 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -33,7 +33,9 @@ repos: hooks: - id: codespell # iTerm is a terminal emulator, named in the hyperlink support probe. - args: [--ignore-words-list=iterm] + # "sting" is a real verb, listed in NON_IMPERATIVE_LOOKALIKES. + # Quoted: an unquoted comma splits the flow sequence into two args. + args: ["--ignore-words-list=iterm,sting"] - repo: https://github.com/commit-check/commit-check rev: v2.11.0 hooks: diff --git a/commit_check/engine.py b/commit_check/engine.py index c67cd5dc..62dabf57 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -24,7 +24,7 @@ git_merge_base, git_rev_parse_verify, ) -from commit_check.imperatives import IMPERATIVES +from commit_check.imperatives import IMPERATIVES, NON_IMPERATIVE_LOOKALIKES class ValidationResult(IntEnum): @@ -392,7 +392,20 @@ def _validate_subject(self, subject: str) -> ValidationResult: class SubjectImperativeValidator(SubjectValidator): - """Validates that subject uses imperative mood.""" + """Validates that subject uses imperative mood. + + Decides on the first word's form rather than its membership in a + vocabulary: a past tense ("fixed"), a gerund ("adding") or a third person + ("fixes") is not imperative, and nothing else disqualifies. A list can + only reject correct subjects wherever it falls short, and it always does: + on 59k strictly imperative subjects from git.git it rejected 45%, against + 1% here. See #526. + + The loosening is deliberate: a noun-led subject ("parser improvements") + now passes, where the list rejected it by accident of vocabulary. + """ + + _INFLECTED = ("ed", "ing") def _validate_subject(self, subject: str) -> ValidationResult: # Skip merge commits and fixup commits @@ -408,11 +421,40 @@ def _validate_subject(self, subject: str) -> ValidationResult: return ValidationResult.PASS first_word = match.group(1).lower() - if first_word in IMPERATIVES: - return ValidationResult.PASS - self._print_failure(subject) - return ValidationResult.FAIL + if self._is_inflected(first_word): + self._print_failure(subject) + return ValidationResult.FAIL + + return ValidationResult.PASS + + @classmethod + def _is_inflected(cls, word: str) -> bool: + """Whether *word* carries past-tense, gerund or third-person marking.""" + if word in NON_IMPERATIVE_LOOKALIKES: + return False + if word.endswith(cls._INFLECTED): + return True + return cls._is_third_person(word) + + @classmethod + def _is_third_person(cls, word: str) -> bool: + """Whether *word* is a verb wearing the third-person singular -s. + + Unlike -ed and -ing, a trailing -s is weak evidence on its own -- + plural nouns wear one too ("status report") -- so it asks for + corroboration: the stem has to be a verb we already know. "tests" is + genuinely both, and this reads it as the verb. + """ + # "address" and "process" end in -ss without being third person. + if not word.endswith("s") or word.endswith("ss"): + return False + stems = {word[:-1]} + if word.endswith("es"): + stems.add(word[:-2]) + if word.endswith("ies"): + stems.add(word[:-3] + "y") + return any(stem in IMPERATIVES for stem in stems) class SubjectLengthValidator(SubjectValidator): diff --git a/commit_check/imperatives.py b/commit_check/imperatives.py index f3af9e08..ee97faee 100644 --- a/commit_check/imperatives.py +++ b/commit_check/imperatives.py @@ -2,17 +2,15 @@ # https://github.com/crate-ci/imperative/blob/master/assets/imperatives.txt # and extended since. # -# Some of these are more commonly encountered as nouns, but leaving them out -# rejects a subject that is written correctly, which is the worse failure: the -# contributor has to reword something that was never wrong, and the only way -# they learn which words are acceptable is by trial and error. +# This is NOT an allow-list, and a subject is never rejected for being absent +# from it. CC003 decides on the word's form (see SubjectImperativeValidator), +# and the only thing this set is still consulted for is the stem test behind +# the third-person -s rule: "fixes" is a verb because "fix" is in here, while +# "status" is not, because dropping its -s leaves nothing this set contains. # -# For the same reason both spellings of every -ize/-ise verb are listed. A -# project writing British English is not making a mistake. -# -# Additions are welcome and cheap. The list can only ever approximate "is this -# an English imperative verb", so treat a rejected-but-correct subject as a bug -# in this file rather than as something the author should work around. +# So a missing verb costs a missed violation, never a false rejection -- there +# is nothing to keep up with, and no need to send a patch adding the verb you +# just used. Additions still help the -s rule catch more, and cost nothing. IMPERATIVES = { "abort", @@ -44,7 +42,6 @@ "authenticate", "authorise", "authorize", - "auto", "automate", "avoid", "await", @@ -304,7 +301,6 @@ "parameterise", "parameterize", "parse", - "partial", "pass", "pause", "perform", @@ -441,7 +437,6 @@ "serve", "set", "settle", - "setup", "shard", "shorten", "show", @@ -545,3 +540,50 @@ "yield", "zip", } + + +# Imperative verbs that end in -ed or -ing without being a past tense or a +# gerund, and so would be misread by the morphology rule in +# SubjectImperativeValidator. Short enough to enumerate, which is what lets +# that rule work on shape instead of on a vocabulary of every English verb. +# +# Both groups are meant to be the whole family rather than a sample of it -- +# a half-enumerated closed set would put back exactly the "add my word" +# treadmill this check was rebuilt to escape. Err towards including a word: +# an entry that never comes up costs nothing, a missing one rejects someone +# who wrote correct English. +NON_IMPERATIVE_LOOKALIKES = { + # -ed, but not a past tense + "bleed", + "breed", + "embed", + "exceed", + "feed", + "heed", + "need", + "proceed", + "seed", + "shed", + "shred", + "speed", + "spread", + "succeed", + "wed", + "weed", + # -ing, but not a gerund + "bring", + "cling", + "ding", + "fling", + "ping", + "ring", + "sing", + "sling", + "spring", + "sting", + "string", + "swing", + "wing", + "wring", + "zing", +} diff --git a/tests/engine_test.py b/tests/engine_test.py index 3f3ec867..ffba921a 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -2560,3 +2560,117 @@ def test_commit_type_rule_still_fails_for_other_authors(self): patch("commit_check.engine.get_commit_info", return_value="Ada Lovelace"), ): assert validator.validate(context) == ValidationResult.FAIL + + +class TestImperativeMorphology: + """The rule decides on form, not on vocabulary. See #526. + + A word list can only approximate "is this an English imperative", and the + cost of every gap falls on someone who wrote the subject correctly. These + pin the three things that replace it: inflected first words are rejected, + everything else is not, and the few words whose spelling trips the test + are named rather than guessed at. + """ + + def _verdict(self, subject): + validator = SubjectImperativeValidator( + ValidationRule(check="subject_imperative") + ) + return validator.validate(ValidationContext(stdin_text=subject)) + + @pytest.mark.benchmark + @pytest.mark.parametrize( + "subject", + [ + "fix: fixed the parser", # past tense + "fix: updated the docs", + "fix: removed the flag", + "feat: adding a retry", # gerund + "feat: implementing the cache", + "fix: fixes the parser", # third person + "fix: removes the flag", + "docs: documents the API", + "fix: tries the fallback", # -ies, so the stem is "try" + "fix: applies the patch", + ], + ) + def test_inflected_first_words_are_rejected(self, subject): + """The failure the rule exists for, and now the only one it reports.""" + assert self._verdict(subject) == ValidationResult.FAIL + + @pytest.mark.benchmark + @pytest.mark.parametrize( + "subject", + [ + # Correct imperatives that no list contained, or ever would. + "feat: reword the report format", + "refactor: dedupe the helper", + "chore: decommission the legacy path", + "fix: loosen the guard", + "fix: backfill an absent value", + # Adverb-led subjects, which a list of *verbs* cannot represent + # without becoming a list of not-verbs. + "feat: optionally skip the hook", + "fix: explicitly close the handle", + # British spelling, which is not a mistake. + "refactor: normalise the path separators", + ], + ) + def test_uninflected_first_words_pass_without_being_listed(self, subject): + assert self._verdict(subject) == ValidationResult.PASS + + @pytest.mark.benchmark + @pytest.mark.parametrize( + "subject", + [ + "fix: embed the token", # -ed, not a past tense + "fix: need a newer pip", + "fix: proceed without the cache", + "chore: spread the load", + "feat: bring back the flag", # -ing, not a gerund + "fix: string the parts together", + "fix: ping the endpoint", + "chore: weed out the dead code", + "fix: heed the configured timeout", + "feat: sling the payload over the wire", + ], + ) + def test_lookalikes_are_not_mistaken_for_inflection(self, subject): + """The words a suffix rule gets wrong, enumerated rather than guessed.""" + assert self._verdict(subject) == ValidationResult.PASS + + @pytest.mark.benchmark + @pytest.mark.parametrize( + "subject", + [ + "fix: address the warning", # -ss, never third person + "fix: process the queue", + "fix: guess the encoding", + "fix: focus the search", # -s, but the stem is not a verb + "fix: status report is empty", # noun-led + "chore: deps bump", + # Stems that are not verbs at all, so their plurals are nouns. + "fix: partials are rendered twice", + "chore: setups differ between CI and local", + "fix: always quote the path", # adverb-led + "fix: sometimes the cache is stale", + ], + ) + def test_single_s_words_are_not_assumed_third_person(self, subject): + """A trailing -s only counts when the stem is a verb we know.""" + assert self._verdict(subject) == ValidationResult.PASS + + @pytest.mark.benchmark + def test_a_noun_led_subject_now_passes(self): + """Documents the deliberate loosening, so a change to it is a choice. + + The old list rejected this by accident of vocabulary, not because it + detected the mood. Naming it here means anyone tightening it later + does so on purpose. + """ + assert self._verdict("fix: parser improvements") == ValidationResult.PASS + + @pytest.mark.benchmark + def test_merge_and_fixup_subjects_still_bypass_the_rule(self): + assert self._verdict("Merge branch 'main' into topic") == ValidationResult.PASS + assert self._verdict("fixup! fixed the parser") == ValidationResult.PASS