From 06111ce2d880dacde85a935dde4d3427ad5067c0 Mon Sep 17 00:00:00 2001 From: Thomas Schmelzer Date: Thu, 8 Oct 2026 18:01:49 +0400 Subject: [PATCH] refactor: define ts and behind_count once, in forge.py (#92) --- collector/jq_collector/forge.py | 43 +++++++++++++++++++++++ collector/jq_collector/github.py | 22 ++++-------- collector/jq_collector/github_api.py | 2 +- collector/jq_collector/github_payloads.py | 19 +--------- collector/jq_collector/gitlab.py | 43 ++++------------------- collector/tests/test_github_http.py | 9 ++--- collector/tests/test_gitlab.py | 12 +++---- 7 files changed, 69 insertions(+), 81 deletions(-) diff --git a/collector/jq_collector/forge.py b/collector/jq_collector/forge.py index af0df10..a372671 100644 --- a/collector/jq_collector/forge.py +++ b/collector/jq_collector/forge.py @@ -20,6 +20,7 @@ import concurrent.futures import logging from collections.abc import Callable +from datetime import datetime from typing import Any, Protocol, TypedDict from .state import RemoteRepo, WorkflowRun @@ -32,6 +33,48 @@ # or red, so they are left out of the exposition rather than counted as failures. INCONCLUSIVE_CONCLUSIONS = frozenset({"cancelled", "stale"}) + +def ts(value: str | None) -> float: + """An ISO timestamp as epoch seconds, or 0.0 when there is none to read. + + One function for both forges. GitLab writes ``2026-08-31T06:05:36.000Z`` + where GitHub has no milliseconds; both parse as they stand, because 3.11 + taught ``fromisoformat`` the whole of ISO 8601, trailing ``Z`` included, and + 3.11 is this package's floor. + + >>> ts("1970-01-01T00:01:00+00:00") + 60.0 + >>> ts("1970-01-01T00:01:00.000Z") + 60.0 + >>> ts(None), ts("not a date") + (0.0, 0.0) + """ + if not value: + return 0.0 + try: + return datetime.fromisoformat(value).timestamp() + except ValueError: + return 0.0 + + +def behind_count(tags: list[str], ref: str) -> int | None: + """How many template releases were published after ``ref``, newest first in ``tags``. + + None when the pinned ref is not a published release tag - a branch name or + a sha - so the dashboard can show "unknown" instead of "current". The same + for both forges: the template lives on GitHub whichever forge the repo + pinning it lives on, so drift is measured against one tag list. + + >>> behind_count(["v2.0", "v1.9", "v1.8"], "v1.8") + 2 + >>> behind_count(["v2.0"], "main") is None + True + """ + if not ref or ref not in tags: + return None + return tags.index(ref) + + # GitLab's spelling -> the vocabulary the board already speaks. # # `manual` is a job waiting for somebody to press the button - a deploy gate, diff --git a/collector/jq_collector/github.py b/collector/jq_collector/github.py index bd9b2a6..962415d 100644 --- a/collector/jq_collector/github.py +++ b/collector/jq_collector/github.py @@ -28,12 +28,13 @@ from .forge import ( GOOD_CONCLUSIONS, INCONCLUSIVE_CONCLUSIONS, + behind_count, cached_ref, ci_summary, fan_out, + ts, ) from .github_api import GitHub -from .github_payloads import ts as _ts from .state import RemoteRepo, WorkflowRun log = logging.getLogger(__name__) @@ -48,17 +49,6 @@ _MAX_WORKERS = 8 -def _behind_count(tags: list[str], ref: str) -> int | None: - """How many releases were published after ``ref``. - - Returns None when the pinned ref is not a published release tag - a branch - name or a sha - so the dashboard can show "unknown" instead of "current". - """ - if not ref or ref not in tags: - return None - return tags.index(ref) - - def collect( cfg: Config, ref_cache: dict[str, tuple[str, str]], @@ -155,7 +145,7 @@ def _remote_repo( visibility=raw.get("visibility") or "unknown", archived=bool(raw.get("archived")), head_sha=head_sha, - pushed_at=_ts(raw.get("pushed_at")), + pushed_at=ts(raw.get("pushed_at")), protected=protected, required_reviews=required_reviews, allows_force_push=allows_force_push, @@ -163,7 +153,7 @@ def _remote_repo( alerts=tuple(sorted((alerts or {}).items())), rhiza_managed=bool(ref), rhiza_ref=ref, - rhiza_behind=_behind_count(tags, ref), + rhiza_behind=behind_count(tags, ref), **ci_summary(workflows), workflows=workflows, coverage=coverage, @@ -200,12 +190,12 @@ def _coverage( def _workflow_run(run: dict[str, Any]) -> WorkflowRun: """One entry of ``latest_runs`` as the board's WorkflowRun.""" - finished = _ts(run.get("updated_at")) + finished = ts(run.get("updated_at")) return WorkflowRun( name=run.get("_name") or run.get("name") or "unnamed", conclusion=run.get("conclusion") or "", finished_at=finished, - duration=max(0.0, finished - _ts(run.get("run_started_at"))), + duration=max(0.0, finished - ts(run.get("run_started_at"))), url=run.get("html_url") or "", ) diff --git a/collector/jq_collector/github_api.py b/collector/jq_collector/github_api.py index e61325f..1a8f475 100644 --- a/collector/jq_collector/github_api.py +++ b/collector/jq_collector/github_api.py @@ -22,6 +22,7 @@ from . import coverage_xml from .config import Config +from .forge import ts from .github_payloads import ( active_workflow_names, alert_counts, @@ -29,7 +30,6 @@ inconclusive, newest_coverage_artifact, newest_per_workflow, - ts, ) from .state import MergedPull, PullRequest diff --git a/collector/jq_collector/github_payloads.py b/collector/jq_collector/github_payloads.py index 35703e2..6f7d480 100644 --- a/collector/jq_collector/github_payloads.py +++ b/collector/jq_collector/github_payloads.py @@ -8,10 +8,9 @@ from __future__ import annotations -from datetime import datetime from typing import Any -from .forge import INCONCLUSIVE_CONCLUSIONS +from .forge import INCONCLUSIVE_CONCLUSIONS, ts # The artifact CI uploads its coverage report as, and the file to read inside # it. Repos that publish nothing by this name simply have no coverage on the @@ -19,22 +18,6 @@ COVERAGE_ARTIFACT = "coverage-report" -def ts(value: str | None) -> float: - """An ISO timestamp as epoch seconds, or 0.0 when there is none to read. - - >>> ts("1970-01-01T00:01:00+00:00") - 60.0 - >>> ts(None), ts("not a date") - (0.0, 0.0) - """ - if not value: - return 0.0 - try: - return datetime.fromisoformat(value).timestamp() - except ValueError: - return 0.0 - - def active_workflow_names(workflows: list[dict[str, Any]]) -> dict[int, str]: """Active workflow id -> a display name unique among them. diff --git a/collector/jq_collector/gitlab.py b/collector/jq_collector/gitlab.py index 26ddd4f..9dc5a94 100644 --- a/collector/jq_collector/gitlab.py +++ b/collector/jq_collector/gitlab.py @@ -38,7 +38,6 @@ from __future__ import annotations import logging -from datetime import datetime from typing import Any from urllib.parse import quote @@ -46,7 +45,7 @@ import yaml from .config import Config -from .forge import cached_ref, ci_summary, fan_out, normalise_gitlab_status +from .forge import behind_count, cached_ref, ci_summary, fan_out, normalise_gitlab_status, ts from .state import MergedPull, PullRequest, RemoteRepo, WorkflowRun log = logging.getLogger(__name__) @@ -58,22 +57,6 @@ _PER_PAGE = 100 -def _ts(value: str | None) -> float: - """One GitLab timestamp as epoch seconds, or 0.0 if it is unusable. - - GitLab returns ``2026-08-31T06:05:36.000Z``, where GitHub's has no - milliseconds. Both parse as they stand: 3.11 taught ``fromisoformat`` the - whole of ISO 8601, including the trailing ``Z``, and 3.11 is this package's - floor - so this is `github_payloads.ts` with a different docstring. - """ - if not value: - return 0.0 - try: - return datetime.fromisoformat(value).timestamp() - except ValueError: - return 0.0 - - def _pid(full_name: str) -> str: """A project path as GitLab's ``:id`` path segment.""" return quote(full_name, safe="") @@ -286,8 +269,8 @@ def open_merge_requests(self, full_name: str) -> tuple[int, list[PullRequest]]: title=(item.get("title") or "")[:120], author=(item.get("author") or {}).get("username") or "unknown", draft=bool(item.get("draft")), - created_at=_ts(item.get("created_at")), - updated_at=_ts(item.get("updated_at")), + created_at=ts(item.get("created_at")), + updated_at=ts(item.get("updated_at")), checks=_checks_state(status), url=item.get("web_url") or "", ) @@ -306,7 +289,7 @@ def recent_merges(self, full_name: str, limit: int) -> list[MergedPull]: return [] merged = [] for item in raw: - at = _ts(item.get("merged_at")) + at = ts(item.get("merged_at")) if not at: continue merged.append( @@ -376,18 +359,6 @@ def _protection(entry: dict[str, Any] | None) -> tuple[bool | None, bool, int]: return True, bool(entry.get("allow_force_push")), 0 -def _behind_count(tags: list[str], ref: str) -> int | None: - """How many template releases *ref* is behind, or None if it is not one. - - Deliberately identical in meaning to ``github._behind_count``: the template - lives on GitHub whichever forge the repo pinning it lives on, so drift is - measured against the same tag list either way. - """ - if not ref or ref not in tags: - return None - return tags.index(ref) - - def collect( cfg: Config, ref_cache: dict[str, tuple[str, str]], @@ -458,7 +429,7 @@ def _remote_repo( visibility=_visibility(raw), archived=bool(raw.get("archived")), head_sha=head_sha, - pushed_at=_ts(raw.get("last_activity_at")), + pushed_at=ts(raw.get("last_activity_at")), protected=protected, required_reviews=reviews, allows_force_push=force_push, @@ -467,7 +438,7 @@ def _remote_repo( alerts=(), rhiza_managed=bool(ref), rhiza_ref=ref, - rhiza_behind=_behind_count(tags, ref), + rhiza_behind=behind_count(tags, ref), **ci_summary(workflows), workflows=workflows, coverage=_pipeline_coverage(pipeline), @@ -513,7 +484,7 @@ def _pipeline_runs( WorkflowRun( name=job.get("name") or "unnamed", conclusion=normalise_gitlab_status(job.get("status") or ""), - finished_at=_ts(job.get("finished_at")), + finished_at=ts(job.get("finished_at")), duration=float(job.get("duration") or 0.0), url=job.get("web_url") or "", ) diff --git a/collector/tests/test_github_http.py b/collector/tests/test_github_http.py index f3e86c8..e73a598 100644 --- a/collector/tests/test_github_http.py +++ b/collector/tests/test_github_http.py @@ -21,7 +21,8 @@ from test_coverage import StubAPI from jq_collector.config import Config -from jq_collector.github import GitHub, _behind_count, _ts +from jq_collector.forge import behind_count, ts +from jq_collector.github import GitHub def client(handler, **cfg_overrides) -> GitHub: @@ -561,11 +562,11 @@ def test_an_unreadable_closed_feed_yields_no_merges(): @pytest.mark.parametrize("raw", [None, "", "not a date", "2026-13-45T99:99:99Z"]) def test_an_unparseable_timestamp_is_zero(raw): """Zero sorts to the end rather than raising mid-refresh.""" - assert _ts(raw) == 0.0 + assert ts(raw) == 0.0 def test_a_real_timestamp_is_parsed(): - assert _ts("2026-08-01T00:00:00+00:00") > 0 + assert ts("2026-08-01T00:00:00+00:00") > 0 @pytest.mark.parametrize( @@ -581,7 +582,7 @@ def test_a_real_timestamp_is_parsed(): def test_releases_behind_is_none_when_the_ref_is_not_a_release(tags, ref, expected): """A branch or a sha is not "up to date" and not "behind" - it is unknown, and the board says so rather than implying either.""" - assert _behind_count(tags, ref) == expected + assert behind_count(tags, ref) == expected def test_close_releases_the_client(): diff --git a/collector/tests/test_gitlab.py b/collector/tests/test_gitlab.py index 5ec716a..3d53f36 100644 --- a/collector/tests/test_gitlab.py +++ b/collector/tests/test_gitlab.py @@ -13,7 +13,7 @@ import pytest from conftest import job, project -from jq_collector import gitlab +from jq_collector import forge, gitlab from jq_collector.config import Config from jq_collector.forge import normalise_gitlab_status @@ -80,12 +80,12 @@ def test_only_this_forges_share_is_asked_for(make_gitlab): def test_gitlab_timestamps_with_a_z_and_milliseconds_parse(value, expected_year): import datetime - assert datetime.datetime.fromtimestamp(gitlab._ts(value), datetime.UTC).year == expected_year + assert datetime.datetime.fromtimestamp(forge.ts(value), datetime.UTC).year == expected_year @pytest.mark.parametrize("value", [None, "", "not a date"]) def test_an_unusable_timestamp_is_zero_not_a_crash(value): - assert gitlab._ts(value) == 0.0 + assert forge.ts(value) == 0.0 # -- the status vocabulary -------------------------------------------------- @@ -161,15 +161,15 @@ def test_drift_is_measured_against_the_github_template_tags(): drift is the same count against the same list.""" tags = ["v2.0.0", "v1.9.0", "v1.8.0"] - assert gitlab._behind_count(tags, "v2.0.0") == 0 - assert gitlab._behind_count(tags, "v1.8.0") == 2 + assert forge.behind_count(tags, "v2.0.0") == 0 + assert forge.behind_count(tags, "v1.8.0") == 2 @pytest.mark.parametrize("ref", ["", "main", "deadbeef", "v9.9.9"]) def test_a_ref_that_is_not_a_release_is_unknown_not_zero(ref): """Zero would read as "up to date", which is the opposite of "we cannot tell" for a repo pinned to a branch or a sha.""" - assert gitlab._behind_count(["v2.0.0", "v1.9.0"], ref) is None + assert forge.behind_count(["v2.0.0", "v1.9.0"], ref) is None def test_the_template_pointer_is_read_as_yaml_from_the_raw_route(make_gitlab):