Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/commits.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ jobs:
- name: Validate Conventional Commit PR title
env:
PR_TITLE: ${{ github.event.pull_request.title }}
# The squash subject is "<title> (#<number>)", so the title budget
# depends on how long the number is.
PR_NUMBER: ${{ github.event.pull_request.number }}
run: make check-pr-title

messages:
Expand Down
37 changes: 33 additions & 4 deletions scripts/check_pr_title.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,43 @@ def _load_commit_policy() -> ModuleType:
MAX_TITLE_LENGTH = _POLICY.MAX_TITLE_LENGTH
TITLE_RE = _POLICY.TITLE_RE

# Room for the longest suffix a squash merge is likely to append, used when the
# PR number is not known (a local ``make check-pr-title``). CI passes the real
# number and gets an exact budget.
FALLBACK_SUFFIX_LENGTH = len(" (#0000)")

def validate_title(title: str) -> list[str]:

def squash_suffix(pr_number: str | int | None) -> str | None:
"""What a squash merge appends to the title, if the PR number is known.
This repository merges by squash, and GitHub builds that commit's subject
as ``<PR title> (#<number>)``. The subject that lands on ``main`` is
therefore longer than the title the author typed -- which is the only
thing this check used to measure. v1.4.2 shipped a 73-char subject that
way, from a title that was a perfectly legal 66, and every open PR then
failed ``commit messages`` for a commit none of them wrote.
"""
if pr_number is None or not str(pr_number).strip():
return None
return f" (#{str(pr_number).strip()})"


def validate_title(title: str, *, pr_number: str | int | None = None) -> list[str]:
title = title.strip()
if not title:
return ["missing PR title"]

if len(title) > MAX_TITLE_LENGTH:
return [f"PR title is {len(title)} chars; max is {MAX_TITLE_LENGTH}: {title}"]
suffix = squash_suffix(pr_number)
reserved = len(suffix) if suffix is not None else FALLBACK_SUFFIX_LENGTH
budget = MAX_TITLE_LENGTH - reserved

if len(title) > budget:
appended = f'"{suffix}"' if suffix is not None else f"{reserved} chars"
return [
f"PR title is {len(title)} chars; max is {budget}: {title}\n"
f" a squash merge appends {appended}, and the commit subject it "
f"produces may not exceed {MAX_TITLE_LENGTH}"
]

if not TITLE_RE.match(title):
allowed = ", ".join(ALLOWED_TYPES)
Expand All @@ -56,7 +85,7 @@ def _title_from_args_or_env(argv: list[str]) -> str:

def main(argv: list[str] | None = None) -> int:
title = _title_from_args_or_env(sys.argv[1:] if argv is None else argv)
failures = validate_title(title)
failures = validate_title(title, pr_number=os.getenv("PR_NUMBER"))
if failures:
print("Pull request title check failed:")
print("\n".join(failures))
Expand Down
54 changes: 53 additions & 1 deletion tests/unit/test_scripts/test_check_pr_title.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,63 @@ def test_long_pr_title_is_blocked() -> None:
failures = checker.validate_title(title)

assert len(failures) == 1
assert "max is 72" in failures[0]
assert "max is 64" in failures[0]


def test_title_that_only_fits_before_the_squash_suffix_is_blocked() -> None:
"""The v1.4.2 regression: a legal title that squashes into an illegal one.
``fix(lancedb): cap both caches, give queries their own embed budget`` is
66 characters and passed this check. GitHub appended `` (#496)`` on squash
and put a 73-character subject on ``main``, which then failed
``commit messages`` on every open PR until each one rebased past it.
"""
checker = _load_checker()
title = "fix(lancedb): cap both caches, give queries their own embed budget"
assert len(title) == 66

assert checker.validate_title(title, pr_number=None) != []

failures = checker.validate_title(title, pr_number=496)

assert len(failures) == 1
assert "max is 65" in failures[0]
assert '" (#496)"' in failures[0]


def test_title_that_still_fits_with_the_suffix_is_allowed() -> None:
checker = _load_checker()
title = "fix(lancedb): cap both caches, give queries their own budget"
assert len(title) + len(" (#496)") <= checker.MAX_TITLE_LENGTH

assert checker.validate_title(title, pr_number=496) == []


def test_longer_pr_numbers_cost_more_budget() -> None:
"""The reserve is the real suffix, not a guess."""
checker = _load_checker()
title = "fix(lancedb): cap both caches, give queries their own embe budget"
assert len(title) == 65

assert checker.validate_title(title, pr_number=496) == []
assert checker.validate_title(title, pr_number=4096) != []


def test_main_reads_pr_title_environment(monkeypatch) -> None:
checker = _load_checker()
monkeypatch.setenv("PR_TITLE", "docs(readme): polish launch highlights")

assert checker.main([]) == 0


def test_main_reads_pr_number_environment(monkeypatch) -> None:
"""CI passes the number; without it the check falls back to a reserve."""
checker = _load_checker()
monkeypatch.setenv(
"PR_TITLE", "fix(lancedb): cap both caches, give queries their own budget"
)
monkeypatch.setenv("PR_NUMBER", "496")
assert checker.main([]) == 0

monkeypatch.setenv("PR_NUMBER", "496000000")
assert checker.main([]) == 1
Loading