diff --git a/AGENTS.md b/AGENTS.md index f02b3a16..42629790 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -110,16 +110,35 @@ The CLI is registered via `[project.scripts]` in `pyproject.toml`. and leaves the new stats columns as `—`. - `pr_number`, `git_ref`, `last_commit`, `last_updated` (build time, UTC) - `has_schema_changes`, `build_status` (`success` | `failed`) -- `authors_count` (unique commit authors from `merge_base..PR_HEAD`) +- `authors_count` — **seed value only.** Unique contributor *emails* + (mailmap-canonical, authors ∪ committers, minus GitHub's web-flow + identity) over `merge_base..PR_HEAD`, computed by + `count_contributors` in `tools/inject-schema-pr` at **schema-build** + time. It goes stale as soon as commits land without a schema rebuild, + so renderers prefer `stats.contributors.count` and fall back here only + when stats are absent. Do not read it directly. - On failure: `error_message`, `error_log` - **`stats`** (v2 nested block, populated by `bids-schema collect prs`): `_source_head_sha`, `_collected_at`, `_complete`, `_error`; `pr_state`, `pr_created_at`, `pr_updated_at`, `review_decision`; `commits.{count, first_at, last_at}`; + `contributors.{count, authors, committers, by_identity{...}}`; `reviews.{approved, changes_requested, commented, dismissed, pending, total, by_author{...}}`; `comments.{issue_count, review_thread_count, total, first_at, last_at, by_author{...}}`; `review_threads.{total, unresolved, unresolved_active, unresolved_outdated, unresolved_by_author{...}}`. +`contributors` is the authoritative contributor count, refreshed every +collection cycle. It counts distinct *people* across both the author and +the committer of every commit, keyed on GitHub login where GitHub +resolved the commit email to an account and on the lowercased email +otherwise; a second pass folds bare-email identities into a login when +another commit tied that address to an account. GitHub's own web-flow +identity (`noreply@github.com`) and `[bot]` logins are excluded, while +per-user `…@users.noreply.github.com` addresses are kept. Keying on the +*name* — what `git shortlog` groups by — miscounts anyone who has +committed under two spellings, and ignoring the committer misses anyone +who merely landed a patch. + Per-author records under `reviews.by_author` also carry `last_state` (the reviewer's most recent submission state) and `effective_state` (the state of their most recent non-COMMENTED, non-DISMISSED diff --git a/bids_schema/collect/github.py b/bids_schema/collect/github.py index 82e65ab2..2710ce7c 100644 --- a/bids_schema/collect/github.py +++ b/bids_schema/collect/github.py @@ -74,7 +74,11 @@ def _build_preflight_query(pr_numbers: list[int]) -> str: state createdAt updatedAt reviewDecision headRefOid commits(first:100, after:$cCur) { totalCount pageInfo{hasNextPage endCursor} - nodes{commit{committedDate authoredDate author{user{login}}}} + nodes{commit{ + committedDate authoredDate + author{name email user{login}} + committer{name email user{login}} + }} } reviews(first:100, after:$rCur) { totalCount pageInfo{hasNextPage endCursor} @@ -379,13 +383,32 @@ def _paginate_pr(pr_number: int) -> dict: } +def _extract_actor(actor: dict | None) -> dict: + """Flatten a GraphQL ``GitActor`` into ``{name, email, login}``. + + ``user`` is null when GitHub cannot match the commit's email to an + account (unregistered address, or a co-author trailer) — the raw + name/email are what identity resolution falls back to. + """ + actor = actor or {} + return { + "name": actor.get("name"), + "email": actor.get("email"), + "login": ((actor.get("user") or {}).get("login")), + } + + def _extract_commit(node: dict) -> dict: commit = node.get("commit") or {} - author_user = ((commit.get("author") or {}).get("user") or {}) + author = _extract_actor(commit.get("author")) + committer = _extract_actor(commit.get("committer")) return { "authoredDate": commit.get("authoredDate"), "committedDate": commit.get("committedDate"), - "login": author_user.get("login"), + # Kept for backwards compatibility: the *authoring* login. + "login": author["login"], + "author": author, + "committer": committer, } @@ -472,6 +495,89 @@ def _effective_state(states_by_time: list[tuple[str, str]]) -> str | None: return None +#: GitHub's own machine identity on web-UI commits (merges, "Update branch", +#: squash-from-UI). Note this is NOT the per-user noreply form, which lives on +#: ``users.noreply.github.com`` and belongs to a real person. +WEB_FLOW_EMAIL = "noreply@github.com" + + +def _is_machine_identity(actor: dict) -> bool: + """True for GitHub's own commit identity and for App/bot accounts.""" + login = (actor.get("login") or "").strip().lower() + if login.endswith("[bot]") or login == "web-flow": + return True + return (actor.get("email") or "").strip().lower() == WEB_FLOW_EMAIL + + +def resolve_contributors(commits: list[dict]) -> dict: + """Count the distinct *people* behind a PR's commits. + + Both roles count: someone who lands another person's patch (rebase, + squash-merge, applying a suggestion) contributed to the branch even + though ``git shortlog`` — which only reads the author field — never + shows them. + + Identity is keyed on the GitHub login when GitHub resolved the commit + email to an account, and on the lowercased email otherwise. Keying on + the *name* (what ``git shortlog`` groups by) miscounts anyone who has + committed under two spellings: bids-specification PR #2307 has both + "Chris Markiewicz" and "Christopher J. Markiewicz" on one address, so + a name-keyed count reports six contributors where there are five. + + A second pass folds bare-email identities into a login whenever some + other commit tied that same address to an account, so one person split + across resolved and unresolved commits still counts once. + """ + actors: list[tuple[dict, str]] = [] + for commit in commits: + actors.append((commit.get("author") or {}, "authored")) + actors.append((commit.get("committer") or {}, "committed")) + + # Pass 1 — learn every email GitHub itself tied to an account. + email_to_login: dict[str, str] = {} + for actor, _role in actors: + if _is_machine_identity(actor): + continue + login = (actor.get("login") or "").strip() + email = (actor.get("email") or "").strip().lower() + if login and email: + email_to_login.setdefault(email, login.lower()) + + # Pass 2 — assign each actor a stable key and tally the two roles. + by_identity: dict[str, dict] = {} + for actor, role in actors: + if _is_machine_identity(actor): + continue + login = (actor.get("login") or "").strip() + email = (actor.get("email") or "").strip().lower() + name = (actor.get("name") or "").strip() + if login: + key = login.lower() + elif email: + key = email_to_login.get(email, email) + elif name: + key = name.lower() + else: + continue + + record = by_identity.setdefault(key, { + "name": name or None, "login": login or None, "email": email or None, + "authored": 0, "committed": 0, + }) + record[role] += 1 + # Keep the most informative label we have seen for this person. + record["name"] = record["name"] or (name or None) + record["login"] = record["login"] or (login or None) + record["email"] = record["email"] or (email or None) + + return { + "count": len(by_identity), + "authors": sum(1 for r in by_identity.values() if r["authored"]), + "committers": sum(1 for r in by_identity.values() if r["committed"]), + "by_identity": by_identity, + } + + def derive_stats(fetched: dict, source_head_sha: str | None) -> dict: """Turn the paginated raw response into the final ``stats`` block dict.""" commits = fetched["commits"] @@ -575,6 +681,8 @@ def derive_stats(fetched: dict, source_head_sha: str | None) -> dict: "last_at": commit_last, }, + "contributors": resolve_contributors(commits), + "reviews": { "approved": state_counts.get("APPROVED", 0), "changes_requested": state_counts.get("CHANGES_REQUESTED", 0), diff --git a/bids_schema/render/bep_readme.py b/bids_schema/render/bep_readme.py index aaa62fc9..c88dcd8a 100644 --- a/bids_schema/render/bep_readme.py +++ b/bids_schema/render/bep_readme.py @@ -14,10 +14,12 @@ from bids_schema.render import formatters as fmt TABLE_HEADER = ( - "| BEP # | Title | Google Doc | PR # | Authors | Build | " - "Reviews | Comments | Unresolved | BEP registered | Doc registered | Actions |\n" - "| ----- | ----- | ---------- | ---- | ------- | ----- | " - "------- | -------- | ---------- | -------------- | -------------- | ------- |" + "| BEP # | Title | Google Doc | PR # | # Authors | # Commenters | Build | " + "Reviews | Unresolved | Comments | First comment | Last comment | " + "BEP registered | Doc registered | Actions |\n" + "| ----- | ----- | ---------- | ---- | --------- | ------------ | ----- | " + "------- | ---------- | -------- | ------------- | ------------ | " + "-------------- | -------------- | ------- |" ) FOOTER_DEPRECATION_NOTE = ( @@ -47,7 +49,7 @@ def _format_bep_row(bep_number: str, metadata: dict, base_dir: Path | None) -> s doc_link = f"[Doc]({google_doc})" if google_doc else "—" pr_link = f"[{pr_number}]({fmt.pr_url(pr_number)})" if pr_number else "—" - authors_count = str(pr_metadata.get("authors_count", 0)) if pr_metadata else "—" + authors_cell = fmt.format_contributors(pr_metadata) if pr_metadata else "—" build_cell = fmt.format_build_cell(pr_metadata) if pr_metadata else fmt.format_build_indicator("unknown") bep_registered_cell = fmt.format_date(metadata.get("bep_registered")) @@ -61,8 +63,10 @@ def _format_bep_row(bep_number: str, metadata: dict, base_dir: Path | None) -> s ) return ( - f"| {bep_display} | {title} | {doc_link} | {pr_link} | {authors_count} | " - f"{build_cell} | {cells['reviews']} | {cells['comments']} | {cells['unresolved']} | " + f"| {bep_display} | {title} | {doc_link} | {pr_link} | {authors_cell} | " + f"{cells['commenters']} | {build_cell} | " + f"{cells['reviews']} | {cells['unresolved']} | " + f"{cells['comments_count']} | {cells['comments_first']} | {cells['comments_last']} | " f"{bep_registered_cell} | {doc_registered_cell} | {actions} |" ) @@ -110,10 +114,18 @@ def render(bep_records: list[tuple[str, dict]], base_dir: Path | None = None) -> body_lines.extend([ "", - "Column legend: **Reviews** = `approved✅ / changes_requested❌ / commented💬`; " + "Column legend: **# Authors** = distinct people behind the linked PR\u2019s commits, " + "counting both the author and the committer of each commit and keyed on GitHub " + "account rather than on name; **# Commenters** = distinct accounts that commented " + "on that PR. **Reviews** = submitted reviews as " + "`approved✅ / changes_requested❌ / commented💬`, zero-valued components omitted " + "(so `1✅/27💬`, not `1✅/0❌/27💬`); " "**Unresolved** = count of unresolved inline review threads (bolded if > 0); " + "**Comments** / **First comment** / **Last comment** = issue comments plus review threads " + "on the linked PR, count and dates split so each can be sorted independently; " "**BEP registered** = date the BEP entry was first added to `bids-website:data/beps/beps.yml`; " - "**Doc registered** = date a `google_doc` URL was first attached to that entry.", + "**Doc registered** = date a `google_doc` URL was first attached to that entry. " + "See [`PRs/README.md`](../PRs/) for the full per-PR commit statistics.", "", FOOTER_DEPRECATION_NOTE, "", diff --git a/bids_schema/render/formatters.py b/bids_schema/render/formatters.py index 2b5febfe..d498952e 100644 --- a/bids_schema/render/formatters.py +++ b/bids_schema/render/formatters.py @@ -51,34 +51,39 @@ def format_date(iso: str | None) -> str: return dt.strftime("%Y-%m-%d") -def format_date_window(first: str | None, last: str | None) -> str: - """Render two ISO timestamps as ``YYYY-MM-DD → YYYY-MM-DD``. ``—`` if both missing.""" - if not first and not last: - return "—" - return f"{format_date(first)} → {format_date(last)}" +#: ``stats.reviews`` keys rendered by :func:`format_reviews`, in display order. +REVIEW_COMPONENTS = ( + ("approved", "✅"), + ("changes_requested", "❌"), + ("commented", "💬"), +) def format_reviews(reviews: dict | None) -> str: """Render the aggregate review histogram as ``3✅/2❌/7💬``. - Emoji legend intentionally minimal so the column stays narrow. ``—`` - if the record has no stats block at all. + Zero-valued components are omitted so the cell stays scannable — + ``{approved: 1, changes_requested: 0, commented: 27}`` renders as + ``1✅/27💬`` rather than ``1✅/0❌/27💬``. ``—`` if the record has no + stats block at all, ``0`` if every component is zero. """ if not reviews: return "—" - approved = reviews.get("approved", 0) - changes = reviews.get("changes_requested", 0) - commented = reviews.get("commented", 0) - if approved == changes == commented == 0: + parts = [ + f"{reviews.get(key, 0)}{emoji}" + for key, emoji in REVIEW_COMPONENTS + if reviews.get(key, 0) + ] + if not parts: return "0" - return f"{approved}✅/{changes}❌/{commented}💬" + return "/".join(parts) -def format_activity_span(count: int | None, first: str | None, last: str | None) -> str: - """Render `` ()``. ``—`` if count is falsy/absent.""" - if not count: +def format_count(count: int | None) -> str: + """Render a bare count for its own sortable column. ``None`` → ``—``, ``0`` → ``0``.""" + if count is None: return "—" - return f"{count} ({format_date(first)} → {format_date(last)})" + return str(count) def format_unresolved(n: int | None) -> str: @@ -138,34 +143,75 @@ def stale_marker(stats: dict) -> str: # --- Aggregated helpers used by both renderers --------------------------- +#: Keys returned by :func:`format_stats_cells`. Counts and dates are kept in +#: separate cells so each gets its own sortable column in the rendered table +#: (a combined ``47 (2020-01-16 → 2026-05-08)`` cell sorts as a string, which +#: is useless for every ordering one actually wants). +STATS_CELL_KEYS = ( + "reviews", + "unresolved", + "pr_created", + "commits_count", "commits_first", "commits_last", + "comments_count", "comments_first", "comments_last", + "commenters", +) + + def format_stats_cells(stats: dict) -> dict[str, str]: - """Return the four stats-derived table cells (reviews / comments / - unresolved / commit window) from a PR ``stats`` sub-block. + """Return the stats-derived table cells from a PR ``stats`` sub-block. - Every value defaults to ``—`` if the block is missing or absent — - so v1 records or PRs whose stats haven't been collected yet render - consistently in both READMEs. + Keys are :data:`STATS_CELL_KEYS`. Every value defaults to ``—`` if the + block is missing or absent — so v1 records or PRs whose stats haven't + been collected yet render consistently in both READMEs. + + ``pr_created`` is the PR's own creation date, which is **not** the same + as ``commits_first``: a force-push (rebase, squash, branch recreation) + replaces the branch's commits, so on an old PR the earliest surviving + commit can post-date the PR — and its comments — by years. Showing both + makes that discrepancy legible instead of looking like a bug. """ if not stats: - return {"reviews": "—", "comments": "—", "unresolved": "—", "commit_window": "—"} + return dict.fromkeys(STATS_CELL_KEYS, "—") comments = stats.get("comments") or {} threads = stats.get("review_threads") or {} commits = stats.get("commits") or {} return { - "reviews": format_reviews(stats.get("reviews")), - "comments": format_activity_span( - comments.get("total"), - comments.get("first_at"), - comments.get("last_at"), - ), - "unresolved": format_unresolved(threads.get("unresolved")), - "commit_window": format_date_window( - commits.get("first_at"), - commits.get("last_at"), - ), + "reviews": format_reviews(stats.get("reviews")), + "unresolved": format_unresolved(threads.get("unresolved")), + "pr_created": format_date(stats.get("pr_created_at")), + "commits_count": format_count(commits.get("count")), + "commits_first": format_date(commits.get("first_at")), + "commits_last": format_date(commits.get("last_at")), + "comments_count": format_count(comments.get("total")), + "comments_first": format_date(comments.get("first_at")), + "comments_last": format_date(comments.get("last_at")), + "commenters": format_count(len(comments["by_author"]) + if "by_author" in comments else None), } +def format_contributors(record: dict) -> str: + """The ``# Authors`` cell: how many distinct people are behind the commits. + + Prefers ``stats.contributors.count``, which the collector recomputes on + every cycle from both the author and committer of every commit, keyed on + GitHub login / email. + + Falls back to the record's top-level ``authors_count``, which is a + ``git shortlog -sn | wc -l`` taken once when the schema was *built*. That + value goes stale the moment new commits land without a schema rebuild — + PR #2307 sat at ``2`` from May while the branch grew to five + contributors — and it counts only authors, grouped by name. So it is a + seed value, never preferred over collected stats. + """ + contributors = (stats_of(record).get("contributors") or {}) + count = contributors.get("count") + if count is not None: + return format_count(count) + fallback = record.get("authors_count") + return format_count(fallback) if fallback else "—" + + def format_build_cell(record: dict) -> str: """Build indicator + optional stale marker for a record's ``stats`` block.""" return f"{format_build_indicator(build_status_of(record))}{stale_marker(stats_of(record))}" diff --git a/bids_schema/render/pr_readme.py b/bids_schema/render/pr_readme.py index 06fd238c..f1e5edd2 100644 --- a/bids_schema/render/pr_readme.py +++ b/bids_schema/render/pr_readme.py @@ -20,10 +20,12 @@ from bids_schema.render import formatters as fmt TABLE_HEADER = ( - "| PR # | Authors | Build | Reviews | Comments | Unresolved | " - "Commit window | Last commit | Actions |\n" - "| ---- | ------- | ----- | ------- | -------- | ---------- | " - "------------- | ----------- | ------- |" + "| PR # | # Authors | # Commenters | Build | Created | Reviews | Unresolved | " + "Commits | First commit | Last commit | " + "Comments | First comment | Last comment | Head | Actions |\n" + "| ---- | --------- | ------------ | ----- | ------- | ------- | ---------- | " + "------- | ------------ | ----------- | " + "-------- | ------------- | ------------ | ---- | ------- |" ) @@ -32,14 +34,14 @@ def _format_pr_row(pr_number: str, metadata: dict) -> str: cells = fmt.format_stats_cells(stats) pr_link = f"[{pr_number}]({fmt.pr_url(pr_number)})" - authors_count = str(metadata.get("authors_count", 0)) + authors_cell = fmt.format_contributors(metadata) build_cell = fmt.format_build_cell(metadata) - last_commit_raw = metadata.get("last_commit", "Unknown") - if last_commit_raw and last_commit_raw != "Unknown": - last_commit_cell = f"[{last_commit_raw[:10]}]({fmt.commit_url(last_commit_raw)})" + head_raw = metadata.get("last_commit", "Unknown") + if head_raw and head_raw != "Unknown": + head_cell = f"[{head_raw[:10]}]({fmt.commit_url(head_raw)})" else: - last_commit_cell = "—" + head_cell = "—" actions = fmt.format_actions_cell( "PRs", pr_number, @@ -48,9 +50,11 @@ def _format_pr_row(pr_number: str, metadata: dict) -> str: ) return ( - f"| {pr_link} | {authors_count} | {build_cell} | " - f"{cells['reviews']} | {cells['comments']} | {cells['unresolved']} | " - f"{cells['commit_window']} | {last_commit_cell} | {actions} |" + f"| {pr_link} | {authors_cell} | {cells['commenters']} | {build_cell} | " + f"{cells['pr_created']} | {cells['reviews']} | {cells['unresolved']} | " + f"{cells['commits_count']} | {cells['commits_first']} | {cells['commits_last']} | " + f"{cells['comments_count']} | {cells['comments_first']} | {cells['comments_last']} | " + f"{head_cell} | {actions} |" ) @@ -88,10 +92,28 @@ def render(pr_records: list[tuple[str, dict]]) -> str: body_lines.extend([ "", - "Column legend: **Reviews** = `approved✅ / changes_requested❌ / commented💬`; " + "Column legend: **# Authors** = distinct people behind the commits on the PR " + "branch, counting both the author and the committer of each commit and keyed on " + "GitHub account (so one person who has committed under two name spellings, or who " + "landed someone else\u2019s patch, is counted once \u2014 unlike `git shortlog`, which " + "reads only the author field and groups by name); GitHub\u2019s own web-flow identity " + "and `[bot]` accounts are excluded. " + "**# Commenters** = distinct accounts that left an issue comment or opened a review " + "thread. **Created** = when the PR was opened; " + "**Reviews** = submitted reviews as `approved✅ / changes_requested❌ / commented💬`, " + "zero-valued components omitted (so `1✅/27💬`, not `1✅/0❌/27💬`); " "**Unresolved** = count of unresolved inline review threads (bolded if > 0); " - "**Commit window** = first → last commit dates on the PR branch. " - "An empty cell (`—`) means the stats block hasn't been collected yet.", + "**Commits** / **First commit** / **Last commit** = number of commits currently on the " + "PR branch and their author-date range; **Comments** / **First comment** / **Last comment** = " + "issue comments plus review threads and their date range; **Head** = the commit the schema " + "here was built from. Counts and dates are separate columns so each can be sorted " + "independently. An empty cell (`—`) means the stats block hasn't been collected yet.", + "", + "> **Why can `First comment` predate `First commit`?** The commit columns describe the " + "commits *currently* on the PR branch. A force-push (rebase, squash, branch recreation) " + "replaces them, and GitHub only reports what survived — so on a long-lived PR the " + "earliest surviving commit can post-date the PR itself by years. `Created` is the PR's " + "real start date; comments are never older than that.", "", "## How to Use PR Schemas", "", diff --git a/bids_schema/tests/test_collect_github.py b/bids_schema/tests/test_collect_github.py index 6a12575f..57b0679c 100644 --- a/bids_schema/tests/test_collect_github.py +++ b/bids_schema/tests/test_collect_github.py @@ -60,8 +60,14 @@ def test_derive_stats_end_to_end() -> None: "headRefOid": "abc123", }, "commits": [ - {"authoredDate": "2020-01-15T09:12:03Z", "committedDate": "2020-01-15T09:12:03Z", "login": "alice"}, - {"authoredDate": "2026-05-11T15:27:31Z", "committedDate": "2026-05-11T15:27:31Z", "login": "bob"}, + {"authoredDate": "2020-01-15T09:12:03Z", "committedDate": "2020-01-15T09:12:03Z", + "login": "alice", + "author": {"name": "Alice", "email": "alice@example.org", "login": "alice"}, + "committer": {"name": "Alice", "email": "alice@example.org", "login": "alice"}}, + {"authoredDate": "2026-05-11T15:27:31Z", "committedDate": "2026-05-11T15:27:31Z", + "login": "bob", + "author": {"name": "Bob", "email": "bob@example.org", "login": "bob"}, + "committer": {"name": "Alice", "email": "alice@example.org", "login": "alice"}}, ], "reviews": [ {"state": "APPROVED", "submittedAt": "2025-02-01T10:00:00Z", "login": "alice"}, @@ -111,6 +117,12 @@ def test_derive_stats_end_to_end() -> None: assert stats["commits"]["first_at"] == "2020-01-15T09:12:03Z" assert stats["commits"]["last_at"] == "2026-05-11T15:27:31Z" + # contributors — alice authored one and committed both, bob authored one + assert stats["contributors"]["count"] == 2 + assert stats["contributors"]["authors"] == 2 + assert stats["contributors"]["committers"] == 1 + assert stats["contributors"]["by_identity"]["alice"]["committed"] == 2 + # reviews aggregate assert stats["reviews"]["approved"] == 2 assert stats["reviews"]["commented"] == 1 @@ -445,3 +457,113 @@ def fake_run(query, variables): assert [c["login"] for c in fetched["commits"]] == ["a", "b", "c"] assert fetched["_complete"] is True assert call_ix["i"] == 2 # first + second page + + +# --- contributor identity resolution ------------------------------------ + + +def _commit(author, committer=None): + """Build the shape `_extract_commit` produces. Actors are (name, email, login).""" + def actor(a): + if a is None: + return {"name": None, "email": None, "login": None} + name, email, login = a + return {"name": name, "email": email, "login": login} + return {"author": actor(author), "committer": actor(committer)} + + +@pytest.mark.ai_generated +def test_resolve_contributors_counts_committers_too() -> None: + """Landing someone else's patch is a contribution `git shortlog` never shows.""" + commits = [ + _commit(("Alice", "alice@example.org", "alice"), + ("Bob", "bob@example.org", "bob")), + ] + out = github.resolve_contributors(commits) + assert out["count"] == 2 + assert out["authors"] == 1 + assert out["committers"] == 1 + assert out["by_identity"]["alice"]["authored"] == 1 + assert out["by_identity"]["bob"]["committed"] == 1 + + +@pytest.mark.ai_generated +def test_resolve_contributors_folds_two_name_spellings() -> None: + """The real bids-specification#2307 case: one address, two display names. + + `git shortlog -sn | wc -l` reports 6 contributors there; keying on the + account/address gives the correct 5. + """ + commits = [ + _commit(("Chris Markiewicz", "markiewicz@stanford.edu", "effigies")), + _commit(("Christopher J. Markiewicz", "markiewicz@stanford.edu", "effigies")), + ] + out = github.resolve_contributors(commits) + assert out["count"] == 1 + assert out["by_identity"]["effigies"]["authored"] == 2 + + +@pytest.mark.ai_generated +def test_resolve_contributors_folds_unresolved_email_into_login() -> None: + """Same person, one commit GitHub matched to an account and one it did not.""" + commits = [ + _commit(("Chris Markiewicz", "markiewicz@stanford.edu", "effigies")), + _commit(("Christopher J. Markiewicz", "markiewicz@stanford.edu", None)), + ] + out = github.resolve_contributors(commits) + assert out["count"] == 1 + assert out["by_identity"]["effigies"]["authored"] == 2 + + +@pytest.mark.ai_generated +def test_resolve_contributors_excludes_github_web_flow_and_bots() -> None: + commits = [ + _commit(("Alice", "alice@example.org", "alice"), + ("GitHub", "noreply@github.com", None)), + _commit(("dependabot[bot]", "x@example.org", "dependabot[bot]")), + ] + out = github.resolve_contributors(commits) + assert out["count"] == 1 + assert set(out["by_identity"]) == {"alice"} + + +@pytest.mark.ai_generated +def test_resolve_contributors_keeps_per_user_noreply_addresses() -> None: + """`…@users.noreply.github.com` is a real person, unlike `noreply@github.com`.""" + commits = [ + _commit(("Cody Baker", "51133164+CodyCBakerPhD@users.noreply.github.com", None)), + ] + out = github.resolve_contributors(commits) + assert out["count"] == 1 + + +@pytest.mark.ai_generated +def test_resolve_contributors_empty() -> None: + out = github.resolve_contributors([]) + assert out == {"count": 0, "authors": 0, "committers": 0, "by_identity": {}} + + +@pytest.mark.ai_generated +def test_extract_commit_captures_both_actors() -> None: + node = {"commit": { + "authoredDate": "2020-01-01T00:00:00Z", + "committedDate": "2020-01-02T00:00:00Z", + "author": {"name": "Alice", "email": "alice@example.org", + "user": {"login": "alice"}}, + "committer": {"name": "Bob", "email": "bob@example.org", + "user": {"login": "bob"}}, + }} + out = github._extract_commit(node) + assert out["login"] == "alice" # back-compat: authoring login + assert out["author"]["login"] == "alice" + assert out["committer"]["login"] == "bob" + assert out["committer"]["email"] == "bob@example.org" + + +@pytest.mark.ai_generated +def test_extract_commit_tolerates_null_actors() -> None: + """GraphQL returns null `user` for unregistered emails, and null actors exist.""" + out = github._extract_commit({"commit": {"authoredDate": "x", "committedDate": "y"}}) + assert out["author"] == {"name": None, "email": None, "login": None} + assert out["committer"] == {"name": None, "email": None, "login": None} + assert github.resolve_contributors([out])["count"] == 0 diff --git a/bids_schema/tests/test_render_bep_readme.py b/bids_schema/tests/test_render_bep_readme.py index af9bef23..5ee4fde2 100644 --- a/bids_schema/tests/test_render_bep_readme.py +++ b/bids_schema/tests/test_render_bep_readme.py @@ -9,6 +9,20 @@ from bids_schema.render import bep_readme +#: Column order of the rendered BEP table, for name-based cell assertions. +COLUMNS = [ + "BEP #", "Title", "Google Doc", "PR #", "# Authors", "# Commenters", "Build", + "Reviews", "Unresolved", "Comments", "First comment", "Last comment", + "BEP registered", "Doc registered", "Actions", +] + + +@pytest.mark.ai_generated +def test_table_header_matches_expected_columns() -> None: + header_row = bep_readme.TABLE_HEADER.splitlines()[0] + assert [c.strip() for c in header_row.split("|")[1:-1]] == COLUMNS + + @pytest.mark.ai_generated def test_empty_bep_list_still_renders() -> None: body = bep_readme.render([]) @@ -29,8 +43,10 @@ def test_render_to_disk_joins_pr_stats(base_dir: Path, make_pr, make_bep) -> Non "_error": None, "reviews": {"approved": 3, "changes_requested": 0, "commented": 1}, "comments": {"total": 12, "first_at": "2020-01-16T00:00:00Z", - "last_at": "2026-05-08T00:00:00Z"}, + "last_at": "2026-05-08T00:00:00Z", + "by_author": {"a": {}, "b": {}, "c": {}}}, "review_threads": {"unresolved": 2}, + "contributors": {"count": 4, "authors": 3, "committers": 2}, } pr_path.write_text(json.dumps(pr_data)) @@ -48,9 +64,18 @@ def test_render_to_disk_joins_pr_stats(base_dir: Path, make_pr, make_bep) -> Non # Google Doc link rendered assert "[Doc]" in body # PR stats joined from sibling PR_METADATA.json - assert "3✅/0❌/1💬" in body - assert "**2**" in body - assert "12 (2020-01-16 → 2026-05-08)" in body + row = next(line for line in body.splitlines() if "[011]" in line) + # Zero-valued changes_requested component dropped from the row (the legend + # still mentions `0❌` as a counter-example, so assert on the row only). + assert "3✅/1💬" in row + assert "0❌" not in row + assert "**2**" in row + cells = dict(zip(COLUMNS, (c.strip() for c in row.split("|")[1:-1]))) + assert cells["Comments"] == "12" + assert cells["First comment"] == "2020-01-16" + assert cells["Last comment"] == "2026-05-08" + assert cells["# Authors"] == "4" # collected contributors, not authors_count + assert cells["# Commenters"] == "3" @pytest.mark.ai_generated diff --git a/bids_schema/tests/test_render_formatters.py b/bids_schema/tests/test_render_formatters.py index 72b340b6..c7191c1b 100644 --- a/bids_schema/tests/test_render_formatters.py +++ b/bids_schema/tests/test_render_formatters.py @@ -18,13 +18,6 @@ def test_format_date_iso() -> None: assert fmt.format_date("2026-05-11T15:27:31Z") == "2026-05-11" -@pytest.mark.ai_generated -def test_format_date_window() -> None: - assert fmt.format_date_window(None, None) == "—" - assert fmt.format_date_window("2020-01-15T09:00:00Z", "2026-05-11T15:27:31Z") == \ - "2020-01-15 → 2026-05-11" - - @pytest.mark.ai_generated def test_format_reviews_empty() -> None: assert fmt.format_reviews(None) == "—" @@ -37,6 +30,26 @@ def test_format_reviews_populated() -> None: assert fmt.format_reviews({"approved": 3, "changes_requested": 2, "commented": 7}) == "3✅/2❌/7💬" +@pytest.mark.ai_generated +def test_format_reviews_omits_zero_components() -> None: + """Zero-valued components are dropped rather than rendered as `0✅`.""" + assert fmt.format_reviews({"approved": 1, "changes_requested": 0, "commented": 27}) == "1✅/27💬" + assert fmt.format_reviews({"approved": 0, "changes_requested": 0, "commented": 5}) == "5💬" + assert fmt.format_reviews({"approved": 2, "changes_requested": 0, "commented": 0}) == "2✅" + assert fmt.format_reviews({"approved": 0, "changes_requested": 1, "commented": 8}) == "1❌/8💬" + # Order stays approved → changes_requested → commented regardless of which drop out + assert fmt.format_reviews({"approved": 1, "changes_requested": 2, "commented": 3}) == "1✅/2❌/3💬" + # Missing keys behave like zeros + assert fmt.format_reviews({"commented": 4}) == "4💬" + + +@pytest.mark.ai_generated +def test_format_count() -> None: + assert fmt.format_count(None) == "—" + assert fmt.format_count(0) == "0" + assert fmt.format_count(47) == "47" + + @pytest.mark.ai_generated def test_format_unresolved() -> None: assert fmt.format_unresolved(None) == "—" @@ -44,15 +57,6 @@ def test_format_unresolved() -> None: assert fmt.format_unresolved(6) == "**6**" -@pytest.mark.ai_generated -def test_format_activity_span() -> None: - assert fmt.format_activity_span(None, None, None) == "—" - assert fmt.format_activity_span(0, None, None) == "—" - assert fmt.format_activity_span( - 47, "2020-01-16T10:00:00Z", "2026-05-08T18:12:33Z" - ) == "47 (2020-01-16 → 2026-05-08)" - - @pytest.mark.ai_generated def test_format_build_indicator() -> None: assert fmt.format_build_indicator("success") == "✅" @@ -71,22 +75,77 @@ def test_stale_marker() -> None: @pytest.mark.ai_generated def test_format_stats_cells_empty_returns_all_dashes() -> None: cells = fmt.format_stats_cells({}) - assert cells == {"reviews": "—", "comments": "—", "unresolved": "—", "commit_window": "—"} + assert set(cells) == set(fmt.STATS_CELL_KEYS) + assert set(cells.values()) == {"—"} @pytest.mark.ai_generated def test_format_stats_cells_populated() -> None: cells = fmt.format_stats_cells({ + "pr_created_at": "2018-12-12T18:42:32Z", "reviews": {"approved": 3, "changes_requested": 2, "commented": 7}, "comments": {"total": 47, "first_at": "2020-01-16T00:00:00Z", "last_at": "2026-05-08T00:00:00Z"}, "review_threads": {"unresolved": 6}, - "commits": {"first_at": "2020-01-15T00:00:00Z", "last_at": "2026-05-11T00:00:00Z"}, + "commits": {"count": 14, "first_at": "2020-01-15T00:00:00Z", + "last_at": "2026-05-11T00:00:00Z"}, }) + assert set(cells) == set(fmt.STATS_CELL_KEYS) assert cells["reviews"] == "3✅/2❌/7💬" - assert cells["comments"] == "47 (2020-01-16 → 2026-05-08)" assert cells["unresolved"] == "**6**" - assert cells["commit_window"] == "2020-01-15 → 2026-05-11" + assert cells["pr_created"] == "2018-12-12" + assert cells["commits_count"] == "14" + assert cells["commits_first"] == "2020-01-15" + assert cells["commits_last"] == "2026-05-11" + assert cells["comments_count"] == "47" + assert cells["comments_first"] == "2020-01-16" + assert cells["comments_last"] == "2026-05-08" + assert cells["commenters"] == "—" # no by_author block in this fixture + + +@pytest.mark.ai_generated +def test_format_stats_cells_partial_block_dashes_missing_pieces() -> None: + """A stats block that lacks sub-blocks still yields every key.""" + cells = fmt.format_stats_cells({"reviews": {"approved": 1}}) + assert set(cells) == set(fmt.STATS_CELL_KEYS) + assert cells["reviews"] == "1✅" + assert cells["pr_created"] == "—" + assert cells["commits_count"] == "—" + assert cells["comments_last"] == "—" + + +@pytest.mark.ai_generated +def test_format_stats_cells_commenters() -> None: + cells = fmt.format_stats_cells({ + "comments": {"total": 9, "by_author": {"a": {}, "b": {}, "c": {}}}, + }) + assert cells["commenters"] == "3" + # `by_author` absent (v1 / uncollected) is "not known", not "zero" + assert fmt.format_stats_cells({"comments": {"total": 9}})["commenters"] == "—" + assert fmt.format_stats_cells({"comments": {"by_author": {}}})["commenters"] == "0" + + +@pytest.mark.ai_generated +def test_format_contributors_prefers_collected_stats() -> None: + """stats.contributors.count wins over the stale build-time authors_count.""" + record = { + "authors_count": 2, + "stats": {"contributors": {"count": 5}}, + } + assert fmt.format_contributors(record) == "5" + + +@pytest.mark.ai_generated +def test_format_contributors_falls_back_to_authors_count() -> None: + assert fmt.format_contributors({"authors_count": 2}) == "2" + assert fmt.format_contributors({"authors_count": 0}) == "—" + assert fmt.format_contributors({}) == "—" + + +@pytest.mark.ai_generated +def test_format_contributors_zero_from_stats_is_not_dash() -> None: + """A collected count of 0 is a real answer and must not read as uncollected.""" + assert fmt.format_contributors({"stats": {"contributors": {"count": 0}}}) == "0" @pytest.mark.ai_generated diff --git a/bids_schema/tests/test_render_pr_readme.py b/bids_schema/tests/test_render_pr_readme.py index 04b3ce4c..e382368a 100644 --- a/bids_schema/tests/test_render_pr_readme.py +++ b/bids_schema/tests/test_render_pr_readme.py @@ -16,6 +16,25 @@ def test_empty_pr_list_still_renders() -> None: assert "# BIDS Specification PR Schemas" in body +#: Column order of the rendered PR table, for index-based cell assertions. +COLUMNS = [ + "PR #", "# Authors", "# Commenters", "Build", "Created", "Reviews", "Unresolved", + "Commits", "First commit", "Last commit", + "Comments", "First comment", "Last comment", "Head", "Actions", +] + + +def _cells(row: str) -> list[str]: + """Split a Markdown row into cells (dropping the leading/trailing `|`).""" + return [c.strip() for c in row.split("|")[1:-1]] + + +@pytest.mark.ai_generated +def test_table_header_matches_expected_columns() -> None: + header_row = pr_readme.TABLE_HEADER.splitlines()[0] + assert _cells(header_row) == COLUMNS + + @pytest.mark.ai_generated def test_v1_record_renders_dashes_in_new_columns() -> None: body = pr_readme.render([("518", { @@ -25,18 +44,17 @@ def test_v1_record_renders_dashes_in_new_columns() -> None: "authors_count": 2, })]) row = next(line for line in body.splitlines() if line.startswith("| [518]")) - # Split into cells (drop empty first/last from leading/trailing `|`) - cells = [c.strip() for c in row.split("|")[1:-1]] - # Layout: PR # | Authors | Build | Reviews | Comments | Unresolved | Commit window | Last commit | Actions - assert cells[0].startswith("[518]") - assert cells[1] == "2" - assert cells[2] == "✅" - # New v2 columns: reviews, comments, unresolved, commit window — all "—" for v1 record - assert cells[3] == "—" - assert cells[4] == "—" - assert cells[5] == "—" - assert cells[6] == "—" - assert cells[7].startswith("[8bcb4d678f]") + cells = dict(zip(COLUMNS, _cells(row))) + assert cells["PR #"].startswith("[518]") + # v1 record has no stats, so the build-time authors_count is the fallback + assert cells["# Authors"] == "2" + assert cells["Build"] == "✅" + # Every stats-derived column is "—" for a v1 record + for col in ("# Commenters", "Created", "Reviews", "Unresolved", "Commits", + "First commit", "Last commit", "Comments", "First comment", + "Last comment"): + assert cells[col] == "—", col + assert cells["Head"].startswith("[8bcb4d678f]") @pytest.mark.ai_generated @@ -50,19 +68,84 @@ def test_v2_record_renders_stats() -> None: "stats": { "_complete": True, "_error": None, + "pr_created_at": "2020-06-30T19:44:32Z", "reviews": {"approved": 3, "changes_requested": 2, "commented": 7}, "comments": {"total": 47, "first_at": "2020-01-16T00:00:00Z", - "last_at": "2026-05-08T00:00:00Z"}, + "last_at": "2026-05-08T00:00:00Z", + "by_author": {"a": {}, "b": {}, "c": {}, "d": {}}}, "review_threads": {"unresolved": 6}, "commits": {"count": 14, "first_at": "2020-01-15T09:00:00Z", "last_at": "2026-05-11T15:00:00Z"}, + "contributors": {"count": 5, "authors": 4, "committers": 2}, }, })]) row = next(line for line in body.splitlines() if line.startswith("| [518]")) - assert "3✅/2❌/7💬" in row - assert "**6**" in row - assert "47 (2020-01-16 → 2026-05-08)" in row - assert "2020-01-15 → 2026-05-11" in row + cells = dict(zip(COLUMNS, _cells(row))) + # Collected contributor count supersedes the build-time authors_count of 2 + assert cells["# Authors"] == "5" + assert cells["# Commenters"] == "4" + assert cells["Created"] == "2020-06-30" + assert cells["Reviews"] == "3✅/2❌/7💬" + assert cells["Unresolved"] == "**6**" + assert cells["Commits"] == "14" + assert cells["First commit"] == "2020-01-15" + assert cells["Last commit"] == "2026-05-11" + assert cells["Comments"] == "47" + assert cells["First comment"] == "2020-01-16" + assert cells["Last comment"] == "2026-05-08" + + +@pytest.mark.ai_generated +def test_reviews_cell_drops_zero_components() -> None: + body = pr_readme.render([("352", { + "_schema_version": 2, + "pr_number": "352", + "last_commit": "376e7696b0bbfeb7b2347989beabec5f9833e59e", + "build_status": "success", + "authors_count": 2, + "stats": { + "_complete": True, + "_error": None, + "reviews": {"approved": 1, "changes_requested": 0, "commented": 27}, + }, + })]) + row = next(line for line in body.splitlines() if line.startswith("| [352]")) + assert "1✅/27💬" in row + assert "0❌" not in row + + +@pytest.mark.ai_generated +def test_force_pushed_pr_shows_created_before_first_commit() -> None: + """Regression guard for PR #105: comments predate every surviving commit. + + A force-push replaced the branch's original commits, so ``First commit`` + is years after the PR was opened. ``Created`` must still show the PR's + real start date, which is not later than the first comment. + """ + body = pr_readme.render([("105", { + "_schema_version": 2, + "pr_number": "105", + "last_commit": "fc5f90ce1010d915b4f2d241efc5df1904756276", + "build_status": "success", + "authors_count": 1, + "stats": { + "_complete": True, + "_error": None, + "pr_created_at": "2018-12-12T18:42:32Z", + "comments": {"total": 49, "first_at": "2018-12-12T18:52:00Z", + "last_at": "2024-06-26T12:08:03Z"}, + "commits": {"count": 5, "first_at": "2022-04-22T18:53:57Z", + "last_at": "2023-03-13T20:07:24Z"}, + }, + })]) + row = next(line for line in body.splitlines() if line.startswith("| [105]")) + cells = dict(zip(COLUMNS, _cells(row))) + assert cells["Created"] == "2018-12-12" + assert cells["First comment"] == "2018-12-12" + assert cells["First commit"] == "2022-04-22" + assert cells["Created"] <= cells["First comment"] < cells["First commit"] + # The README explains the discrepancy rather than leaving it looking like a bug + assert "force-push" in body @pytest.mark.ai_generated diff --git a/tools/inject-schema-pr b/tools/inject-schema-pr index a8d6e8ac..111e544e 100755 --- a/tools/inject-schema-pr +++ b/tools/inject-schema-pr @@ -44,6 +44,36 @@ repo=$(readlink -f "$repo") base_dir="$(cd "$(dirname "$0")/.." && pwd)" cd "$base_dir" +# Seed count of distinct people behind a PR's commits, written into +# PR_METADATA.json:authors_count at build time. `bids-schema collect prs` +# later supersedes it with stats.contributors.count, which is refreshed on +# every cron cycle; this value only stands until the first collection. +# +# Both roles count and identity is keyed on the mailmap-canonical *email*, +# not the name: `git shortlog -sn | wc -l` (the previous implementation) +# reads only the author field and groups by name, so it misses anyone who +# merely landed a patch and double-counts anyone who has committed under two +# name spellings. GitHub's own web-flow identity is dropped; per-user +# noreply addresses (`…@users.noreply.github.com`) are real people and kept. +count_contributors() { + local repo="$1" git_ref="$2" merge_base + if [ -z "$repo" ] || [ "$repo" = "unknown" ]; then + echo 0 + return + fi + merge_base=$(git -C "$repo" merge-base "$git_ref" origin/master 2>/dev/null || echo "") + if [ -z "$merge_base" ]; then + echo 0 + return + fi + git -C "$repo" log --format='%aE%n%cE' "$merge_base..$git_ref" 2>/dev/null \ + | tr '[:upper:]' '[:lower:]' \ + | sed '/^$/d' \ + | sort -u \ + | grep -c -v '^noreply@github\.com$' \ + || echo 0 +} + # Handle different types of refs case "$ref_type" in master) @@ -111,14 +141,8 @@ if [ "$BST_EXIT_CODE" -ne 0 ]; then pr_number=$(echo "$output_dir" | sed 's/.*PRs\///') commit_hash=$(git -C "$repo" rev-parse "$git_ref" 2>/dev/null || echo "unknown") - # Count unique authors for the PR - authors_count=0 - if [ -n "$repo" ] && [ "$repo" != "unknown" ]; then - merge_base=$(git -C "$repo" merge-base "$git_ref" origin/master 2>/dev/null || echo "") - if [ -n "$merge_base" ]; then - authors_count=$(git -C "$repo" shortlog -sn "$merge_base..$git_ref" 2>/dev/null | wc -l) - fi - fi + # Seed count of distinct contributors (see count_contributors). + authors_count=$(count_contributors "$repo" "$git_ref") # Delegate PR_METADATA.json emission to the Python side — single # source of truth for schema layout (see design plan §2). @@ -169,13 +193,7 @@ if [ "$generate_metadata" = true ] && [ "$ref_type" = "pr" ]; then commit_hash=$(git -C "$repo" rev-parse "$git_ref" 2>/dev/null || echo "unknown") - authors_count=0 - if [ -n "$repo" ] && [ "$repo" != "unknown" ]; then - merge_base=$(git -C "$repo" merge-base "$git_ref" origin/master 2>/dev/null || echo "") - if [ -n "$merge_base" ]; then - authors_count=$(git -C "$repo" shortlog -sn "$merge_base..$git_ref" 2>/dev/null | wc -l) - fi - fi + authors_count=$(count_contributors "$repo" "$git_ref") # Delegate PR_METADATA.json emission to the Python side — single # source of truth for schema layout (see design plan §2). The