From 1af0b33c501a8495354eef767be1711a9b3e75b7 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Fri, 25 Sep 2026 11:15:19 +1000 Subject: [PATCH] experiment: GitHub digest on PyGithub, Crowdin scripts on crowdin-api-client Both keep this repo's retry, error handling and test fakes; the SDKs supply the endpoints, parameter names and pagination. crowdin-api-client never retries a 429 and retries 5xx with a fixed 100 ms sleep, so crowdin_sdk.py swaps its requester's session for one routed through shared.retry and turns the SDK's own loop off. Sessions are per thread there, which the download and approve scripts now get as well as the report. PyGithub's totalCount on a search is read off the last-page link, which GitHub caps at 1000, so truncation comes from the item count. Search results lazily fetch any attribute missing from the raw JSON, one GET per PR, so the test fixtures carry every field the digest reads. The dry-run payload against the live org is byte-identical to the requests version, at one extra request (GET /orgs/{org}). --- crowdin/approve_strings.py | 51 ++++---- crowdin/crowdin_sdk.py | 89 +++++++++++++ crowdin/download_translations_from_crowdin.py | 56 ++++---- crowdin/report_multiple_translations.py | 68 +++------- crowdin/requirements.txt | 5 +- crowdin/test_download_translations.py | 41 ++++-- crowdin/test_report_multiple_translations.py | 31 ++++- github_prs/digest.py | 120 +++++++---------- github_prs/requirements.txt | 1 + github_prs/test_digest.py | 121 ++++++++++++++---- shared/testing.py | 4 + 11 files changed, 358 insertions(+), 229 deletions(-) create mode 100644 crowdin/crowdin_sdk.py diff --git a/crowdin/approve_strings.py b/crowdin/approve_strings.py index e5613cc..e2b36cb 100644 --- a/crowdin/approve_strings.py +++ b/crowdin/approve_strings.py @@ -34,9 +34,13 @@ import sys import time -import requests +from crowdin_api.api_resources.source_strings.enums import ScopeFilter +from crowdin_api.exceptions import APIException, ValidationError + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +import crowdin_sdk # noqa: E402 -API = "https://api.crowdin.com/api/v2" DEFAULT_PROJECT = "618696" # libsecret attributes identifying the token in the keyring; must match the @@ -128,14 +132,7 @@ def main(): token = get_token() identifiers = args.identifiers - pid = args.project_id - s = requests.Session() - s.headers.update({"Authorization": f"Bearer {token}"}) - - def get(path, **params): - r = s.get(f"{API}{path}", params=params, timeout=30) - r.raise_for_status() - return r.json() + client = crowdin_sdk.client(token, args.project_id, attempts=6, timeout=30) # 1) Resolve identifiers -> string IDs string_ids = {} @@ -143,8 +140,8 @@ def get(path, **params): found = None offset = 0 while True: - data = get(f"/projects/{pid}/strings", filter=ident, - scope="identifier", limit=500, offset=offset)["data"] + data = client.source_strings.list_strings( + filter=ident, scope=ScopeFilter.IDENTIFIER, limit=500, offset=offset)["data"] for row in data: if row["data"]["identifier"] == ident: found = row["data"]["id"] @@ -158,7 +155,7 @@ def get(path, **params): print(f" string '{ident}' -> id {found}") # 2) Target languages - proj = get(f"/projects/{pid}")["data"] + proj = client.projects.get_project()["data"] target_langs = proj["targetLanguageIds"] print(f"\n{len(target_langs)} target languages: {', '.join(target_langs)}\n") @@ -169,15 +166,15 @@ def get(path, **params): approved, skipped, already, errors = 0, 0, 0, 0 for ident, sid in string_ids.items(): for lang in target_langs: - raw = get(f"/projects/{pid}/translations", - stringId=sid, languageId=lang, limit=500)["data"] + raw = client.string_translations.list_string_translations( + stringId=sid, languageId=lang, limit=500)["data"] trans = [t["data"] for t in raw] if args.list: # Show only unapproved translations (i.e. what a run would approve), # honouring the --by-user filter if one was given. - approvals = get(f"/projects/{pid}/approvals", - stringId=sid, languageId=lang, limit=500)["data"] + approvals = client.string_translations.list_translation_approvals( + stringId=sid, languageId=lang, limit=500)["data"] approved_tids = {a["data"]["translationId"] for a in approvals} pending = [t for t in trans if t["id"] not in approved_tids @@ -209,17 +206,19 @@ def get(path, **params): approved += 1 continue - r = s.post(f"{API}/projects/{pid}/approvals", - json={"translationId": tid}, timeout=30) - if r.status_code == 201: + try: + client.string_translations.add_approval(translationId=tid) + except APIException as exc: + message = crowdin_sdk.error_message(exc) + if isinstance(exc, ValidationError) and "already" in message.lower(): + print(f" [ -- ] {ident} / {lang}: already approved {info}") + already += 1 + else: + print(f" [ERR ] {ident} / {lang}: {exc.http_status} {message}") + errors += 1 + else: print(f" [ ok ] {ident} / {lang}: approved {info}") approved += 1 - elif r.status_code == 400 and "already" in r.text.lower(): - print(f" [ -- ] {ident} / {lang}: already approved {info}") - already += 1 - else: - print(f" [ERR ] {ident} / {lang}: {r.status_code} {r.text}") - errors += 1 time.sleep(0.1) # gentle on rate limits if args.list: diff --git a/crowdin/crowdin_sdk.py b/crowdin/crowdin_sdk.py new file mode 100644 index 0000000..feed87c --- /dev/null +++ b/crowdin/crowdin_sdk.py @@ -0,0 +1,89 @@ +"""crowdin-api-client with this repo's retry underneath it. + +The SDK's own retry is a fixed 100 ms sleep, five times, on 5xx only: a 429 raises +at once whatever Retry-After says, and Crowdin throttles at ~40 req/s. So the SDK +supplies the endpoints, parameter names and error mapping, and shared.retry the +transport, by standing in for the requests.Session its requester talks to. +""" +import functools +import json +import threading + +import requests +from crowdin_api import CrowdinClient +from crowdin_api.requester import APIRequester + +from shared.retry import request_with_retry + + +class RetryingSession: + """The SDK's session, routed through shared.retry. + + One requests.Session per thread: the SDK shares its session across callers, and + requests.Session is not guaranteed thread-safe. `fixed` is one session for every + thread instead, which is how a test hands in a fake. + """ + + def __init__(self, attempts): + self.attempts = attempts + self.headers = {} + self.fixed = None + self._local = threading.local() + + def _session(self): + if self.fixed is not None: + return self.fixed + session = getattr(self._local, "session", None) + if session is None: + session = self._local.session = requests.Session() + session.headers.update(self.headers) + return session + + def request(self, method, url, **kwargs): + return request_with_retry(self._session(), method, url, attempts=self.attempts, + **kwargs) + + def close(self): + session = getattr(self._local, "session", None) + if session is not None: + session.close() + + +class RetryingRequester(APIRequester): + def __init__(self, *args, attempts, **kwargs): + super().__init__(*args, **kwargs) + # super().__init__ has already put the auth headers on the session it created. + plain, self._session = self._session, RetryingSession(attempts) + self._session.headers.update(plain.headers) + plain.close() + + +def client(token, project_id, attempts, timeout): + """A CrowdinClient retrying like shared.retry: `attempts` tries, backing off on + 429 and 5xx. max_retries=1 is the SDK's own loop switched off.""" + crowdin = CrowdinClient(token=token, project_id=int(project_id), timeout=timeout, + max_retries=1) + crowdin.API_REQUESTER_CLASS = functools.partial(RetryingRequester, attempts=attempts) + return crowdin + + +def use_session(crowdin, session): + """Answer every request from `session` (a test fake) instead of the network.""" + crowdin.get_api_requestor().session.fixed = session + + +def error_message(exc): + """Crowdin's error message, or the start of the body when it is not that envelope.""" + body = exc.context or b"" + if isinstance(body, bytes): + body = body.decode("utf-8", "replace") + try: + return json.loads(body).get("error", {}).get("message", "Unknown error") + except (ValueError, AttributeError): + return body[:200] or "Unknown error" + + +def fetch_all(resource, method, **params): + """Every item of a paginated list endpoint, unwrapped from the SDK's envelopes.""" + listing = getattr(resource.with_fetch_all(), method) + return [row["data"] for row in listing(**params)["data"]] diff --git a/crowdin/download_translations_from_crowdin.py b/crowdin/download_translations_from_crowdin.py index f39ce23..4e5f9b7 100644 --- a/crowdin/download_translations_from_crowdin.py +++ b/crowdin/download_translations_from_crowdin.py @@ -14,13 +14,15 @@ from threading import Lock, Semaphore import requests +from crowdin_api.api_resources.enums import ExportProjectTranslationFormat +from crowdin_api.exceptions import APIException sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +import crowdin_sdk # noqa: E402 from generate_shared import print_error, print_progress, print_success, run_main # noqa: E402 from shared.retry import request_with_retry # noqa: E402 -API = "https://api.crowdin.com/api/v2" # Crowdin allows 20 simultaneous requests per account. MAX_CONCURRENT_REQUESTS = 20 REQUEST_TIMEOUT_S = 30 @@ -31,35 +33,25 @@ class CrowdinError(Exception): pass -def error_message(response): - """Crowdin's error message, or the start of the body when it is not that envelope.""" - try: - return response.json().get("error", {}).get("message", "Unknown error") - except ValueError: - return response.text[:200] or "Unknown error" - - class Crowdin: """One project's API, gated to Crowdin's concurrency limit across the worker threads.""" def __init__(self, token, project_id, max_workers): self.project_id = project_id - self.session = requests.Session() - self.session.headers["Authorization"] = f"Bearer {token}" + self.api = crowdin_sdk.client(token, project_id, attempts=MAX_ATTEMPTS, + timeout=REQUEST_TIMEOUT_S) # Exports are served from a signed URL on another host, which must not see the token. self.downloads = requests.Session() self.gate = Semaphore(min(max_workers, MAX_CONCURRENT_REQUESTS)) - def request(self, method, path, context, **kwargs): - """One API response's JSON, or CrowdinError naming `context`.""" + def call(self, context, method, **kwargs): + """One SDK call's JSON, or CrowdinError naming `context`.""" with self.gate: - response = request_with_retry(self.session, method, f"{API}/{path}", - attempts=MAX_ATTEMPTS, timeout=REQUEST_TIMEOUT_S, - **kwargs) - if response.status_code != 200: - raise CrowdinError(f"{context}: {error_message(response)} " - f"(Code: {response.status_code})") - return response.json() + try: + return method(**kwargs) + except APIException as exc: + raise CrowdinError(f"{context}: {crowdin_sdk.error_message(exc)} " + f"(Code: {exc.http_status})") from exc def download(self, url, output_path): response = request_with_retry(self.downloads, "GET", url, attempts=MAX_ATTEMPTS, @@ -84,14 +76,12 @@ def export_and_download_language(client, language, directory, is_source, skip_un allow_unapproved): """Export one language and save it as .xliff. Returns the locale.""" locale = language["locale"] - payload = { - "targetLanguageId": language["id"], - "format": "xliff", - "skipUntranslatedStrings": False if is_source else skip_untranslated, - "exportApprovedOnly": False if is_source else not allow_unapproved, - } - export = client.request("POST", f"projects/{client.project_id}/translations/exports", - f"Export failed for {locale}", json=payload) + export = client.call(f"Export failed for {locale}", + client.api.translations.export_project_translation, + targetLanguageId=language["id"], + format=ExportProjectTranslationFormat.XLIFF, + skipUntranslatedStrings=False if is_source else skip_untranslated, + exportApprovedOnly=False if is_source else not allow_unapproved) client.download(export["data"]["url"], os.path.join(directory, f"{locale}.xliff")) return locale @@ -118,8 +108,7 @@ def main(): client = Crowdin(args.api_token, args.project_id, args.max_workers) print_progress("Retrieving project details...") - project = client.request("GET", f"projects/{args.project_id}", - "Failed to retrieve project details") + project = client.call("Failed to retrieve project details", client.api.projects.get_project) if args.verbose: print(json.dumps(project, indent=2)) source_language = project["data"]["sourceLanguage"] @@ -157,10 +146,9 @@ def main(): if args.glossary_id is not None and args.concept_id is not None: print_progress("Retrieving non-translatable strings...") - terms = client.request( - "GET", f"glossaries/{args.glossary_id}/terms", - "Failed to retrieve non-translatable strings", - params={"conceptId": args.concept_id, "limit": 500}) + terms = client.call("Failed to retrieve non-translatable strings", + client.api.glossaries.list_terms, + glossaryId=args.glossary_id, conceptId=args.concept_id, limit=500) if args.verbose: print(json.dumps(terms, indent=2)) with open(os.path.join(args.download_directory, "_non_translatable_strings.json"), "w", diff --git a/crowdin/report_multiple_translations.py b/crowdin/report_multiple_translations.py index f31e586..47f6b7f 100644 --- a/crowdin/report_multiple_translations.py +++ b/crowdin/report_multiple_translations.py @@ -44,15 +44,15 @@ import os import subprocess import sys -import threading import requests +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) -from shared import discord, retry # noqa: E402 +import crowdin_sdk # noqa: E402 +from shared import discord # noqa: E402 from shared.text import clip # noqa: E402 -API = "https://api.crowdin.com/api/v2" DEFAULT_PROJECT = "618696" KEYRING_ATTRS = ["service", "crowdin", "key", "translation-api-token"] @@ -83,27 +83,10 @@ def get_token(cli_token): "or store it via secret-tool).") -def request_with_retry(session, method, url, **kw): - """A Crowdin API call. A non-retryable 4xx raises, so a caller can read the - body of what it asked for without checking the status first.""" - r = retry.request_with_retry(session, method, url, attempts=10, timeout=60, **kw) - r.raise_for_status() - return r - - -def paged(session, path, **params): - """Yield every item from a paginated Crowdin list endpoint.""" - offset = 0 - limit = 500 - while True: - r = request_with_retry(session, "GET", f"{API}{path}", - params={**params, "limit": limit, "offset": offset}) - data = r.json()["data"] - for row in data: - yield row["data"] - if len(data) < limit: - return - offset += limit +def crowdin_client(token, project_id): + """Ten attempts at sixty seconds: a locale is ~1,400 requests at Crowdin's rate + limit, so a 429 partway through is expected rather than exceptional.""" + return crowdin_sdk.client(token, project_id, attempts=10, timeout=60) def user_label(u): @@ -124,30 +107,18 @@ def user_label(u): # full 80-locale scan is ~1366*80 requests ≈ 40 min. To keep a daily job fast we # scan a ROTATING shard of locales each run (see pick_locales), covering them all # over a cycle. Within a locale we fan strings out across a bounded thread pool. -def scan_locale(session, pid, lang, string_ids, strings, editor_url, max_workers): +def scan_locale(client, lang, string_ids, strings, editor_url, max_workers): """Return the finding dicts for one locale (one slot per 2+-translation group).""" approved_here = {} # stringId -> set(translationId) that are approved - for a in paged(session, f"/projects/{pid}/approvals", languageId=lang): + for a in crowdin_sdk.fetch_all(client.string_translations, "list_translation_approvals", + languageId=lang): approved_here.setdefault(a["stringId"], set()).add(a["translationId"]) print(f"[{lang}] scanning {len(string_ids)} strings " f"({len(approved_here)} with approvals) …", file=sys.stderr) - # requests.Session is not guaranteed thread-safe, so instead of sharing the - # caller's session across the pool, give each worker its own (lazily created, - # carrying the same auth header). - tls = threading.local() - - def worker_session(): - s = getattr(tls, "session", None) - if s is None: - s = requests.Session() - s.headers.update(session.headers) - tls.session = s - return s - def process(sid): - trans = list(paged(worker_session(), f"/projects/{pid}/translations", - stringId=sid, languageId=lang)) + trans = crowdin_sdk.fetch_all(client.string_translations, "list_string_translations", + stringId=sid, languageId=lang) approved_tids = approved_here.get(sid, set()) by_cat = collections.defaultdict(list) for t in trans: @@ -198,13 +169,13 @@ def process(sid): return found -def scan(session, pid, locales, strings, editor_url, max_workers): +def scan(client, locales, strings, editor_url, max_workers): """Return a flat list of finding dicts, one per slot with 2+ translations.""" string_ids = list(strings.keys()) findings = [] for lang in locales: - findings.extend(scan_locale(session, pid, lang, string_ids, strings, - editor_url, max_workers)) + findings.extend(scan_locale(client, lang, string_ids, strings, editor_url, + max_workers)) findings.sort(key=lambda f: (f["locale"], f["identifier"] or "", str(f["pluralCategory"]))) return findings @@ -379,13 +350,12 @@ def main(): sys.exit("No Discord webhook (pass --webhook, set DISCORD_WEBHOOK_URL, or use --dry-run).") pid = args.project_id - session = requests.Session() - session.headers.update({"Authorization": f"Bearer {token}"}) + client = crowdin_client(token, pid) # Project details: identifier + per-language editor codes, used to build # editor URLs that actually point at the right locale. (A string's own webUrl # always targets the first target language, regardless of the locale.) - proj = request_with_retry(session, "GET", f"{API}/projects/{pid}").json()["data"] + proj = client.projects.get_project()["data"] project_slug = proj["identifier"] src_code = proj["sourceLanguage"].get("editorCode") or proj["sourceLanguage"]["id"] editor_code = {l["id"]: (l.get("editorCode") or l["id"]) for l in proj["targetLanguages"]} @@ -412,12 +382,12 @@ def editor_url(lang, sid): print(f"Loading strings for project {pid} …", file=sys.stderr) strings = {} - for s in paged(session, f"/projects/{pid}/strings"): + for s in crowdin_sdk.fetch_all(client.source_strings, "list_strings"): strings[s["id"]] = {"identifier": s.get("identifier"), "text": s.get("text")} print(f" {len(strings)} strings; scanning {len(locales)} locale(s): " f"{', '.join(locales)}", file=sys.stderr) - findings = scan(session, pid, locales, strings, editor_url, args.max_workers) + findings = scan(client, locales, strings, editor_url, args.max_workers) print("\n" + "=" * 80, file=sys.stderr) print(f"Found {len(findings)} slot(s) with 2+ translations across " diff --git a/crowdin/requirements.txt b/crowdin/requirements.txt index 6808702..bb0c329 100644 --- a/crowdin/requirements.txt +++ b/crowdin/requirements.txt @@ -2,6 +2,9 @@ babel==2.17.0 certifi==2024.7.4 charset-normalizer==3.3.2 colorama==0.4.6 +crowdin-api-client==1.29.0 +Deprecated==1.3.1 idna==3.7 requests==2.32.3 -urllib3==2.2.2 \ No newline at end of file +urllib3==2.2.2 +wrapt==2.4.1 diff --git a/crowdin/test_download_translations.py b/crowdin/test_download_translations.py index 3a703c0..343e517 100644 --- a/crowdin/test_download_translations.py +++ b/crowdin/test_download_translations.py @@ -1,4 +1,5 @@ """Run with: cd crowdin && python -m unittest discover""" +import json import os import sys import tempfile @@ -8,9 +9,12 @@ sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +import crowdin_sdk # noqa: E402 import download_translations_from_crowdin as download # noqa: E402 from shared.testing import FakeResponse, FakeSession, NoSleep, NonJsonResponse # noqa: E402 +API = "https://api.crowdin.com/api/v2" + class StreamResponse(FakeResponse): def __init__(self, body): @@ -23,36 +27,46 @@ def iter_content(self, chunk_size): def client(api_responses=(), download_responses=()): crowdin = download.Crowdin("secret", "618696", max_workers=4) - crowdin.session = FakeSession(api_responses) + crowdin_sdk.use_session(crowdin.api, FakeSession(api_responses)) crowdin.downloads = FakeSession(download_responses) return crowdin +def api_session(crowdin): + return crowdin.api.get_api_requestor().session + + +def api_calls(crowdin): + return api_session(crowdin).fixed.calls + + class TestRequests(unittest.TestCase): def test_the_token_goes_to_the_api_and_never_to_the_export_host(self): crowdin = download.Crowdin("secret", "618696", max_workers=4) - self.assertEqual(crowdin.session.headers["Authorization"], "Bearer secret") + self.assertEqual(api_session(crowdin).headers["Authorization"], "Bearer secret") self.assertNotIn("Authorization", crowdin.downloads.headers) def test_a_server_error_is_retried(self): crowdin = client([FakeResponse({}, status_code=503), FakeResponse({"data": {"id": 1}})]) with NoSleep(): - self.assertEqual(crowdin.request("GET", "projects/618696", "project")["data"], {"id": 1}) - self.assertEqual(len(crowdin.session.calls), 2) - self.assertEqual(crowdin.session.calls[0][1], f"{download.API}/projects/618696") + self.assertEqual(crowdin.call("project", crowdin.api.projects.get_project)["data"], + {"id": 1}) + self.assertEqual(len(api_calls(crowdin)), 2) + self.assertEqual(api_calls(crowdin)[0][1], f"{API}/projects/618696") def test_a_client_error_names_the_step_and_crowdins_message(self): crowdin = client([FakeResponse({"error": {"message": "Token invalid"}}, status_code=401)]) with self.assertRaises(download.CrowdinError) as caught: - crowdin.request("GET", "projects/618696", "Failed to retrieve project details") + crowdin.call("Failed to retrieve project details", crowdin.api.projects.get_project) self.assertEqual(str(caught.exception), "Failed to retrieve project details: Token invalid (Code: 401)") def test_a_non_json_error_page_still_reads_as_an_error(self): page = NonJsonResponse() page.status_code = 404 + crowdin = client([page]) with self.assertRaises(download.CrowdinError) as caught: - client([page]).request("GET", "projects/618696", "project") + crowdin.call("project", crowdin.api.projects.get_project) self.assertIn("maintenance", str(caught.exception)) self.assertIn("404", str(caught.exception)) @@ -68,11 +82,12 @@ def test_a_language_is_exported_then_saved_under_its_locale(self): with open(os.path.join(directory, "de-DE.xliff"), "rb") as handle: self.assertEqual(handle.read(), b"") self.assertEqual(locale, "de-DE") - method, url, kwargs = crowdin.session.calls[0] - self.assertEqual((method, url), ("POST", f"{download.API}/projects/618696/translations/exports")) - self.assertEqual(kwargs["json"], {"targetLanguageId": "de", "format": "xliff", - "skipUntranslatedStrings": True, - "exportApprovedOnly": True}) + method, url, kwargs = api_calls(crowdin)[0] + self.assertEqual((method.upper(), url), + ("POST", f"{API}/projects/618696/translations/exports")) + self.assertEqual(json.loads(kwargs["data"]), + {"targetLanguageId": "de", "format": "xliff", + "skipUntranslatedStrings": True, "exportApprovedOnly": True}) self.assertEqual(crowdin.downloads.calls[0][1], "https://storage.example/x.xliff") def test_the_source_language_is_always_exported_whole(self): @@ -82,7 +97,7 @@ def test_the_source_language_is_always_exported_whole(self): download.export_and_download_language( crowdin, {"id": "en", "locale": "en"}, directory, is_source=True, skip_untranslated=True, allow_unapproved=False) - payload = crowdin.session.calls[0][2]["json"] + payload = json.loads(api_calls(crowdin)[0][2]["data"]) self.assertFalse(payload["skipUntranslatedStrings"]) self.assertFalse(payload["exportApprovedOnly"]) diff --git a/crowdin/test_report_multiple_translations.py b/crowdin/test_report_multiple_translations.py index 6f8f347..95f58ff 100644 --- a/crowdin/test_report_multiple_translations.py +++ b/crowdin/test_report_multiple_translations.py @@ -7,28 +7,45 @@ import sys import unittest -import requests +from crowdin_api.exceptions import APIException sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +import crowdin_sdk # noqa: E402 import report_multiple_translations as report # noqa: E402 from shared.testing import FakeResponse, FakeSession, NoSleep, Patched # noqa: E402 -class TestRequestWithRetry(unittest.TestCase): +class TestClient(unittest.TestCase): + def client(self, responses): + client = report.crowdin_client("tok", "618696") + session = FakeSession(responses) + crowdin_sdk.use_session(client, session) + return client, session + def test_a_client_error_raises_rather_than_returning(self): """Callers read the body straight off the response, so a 4xx has to stop them.""" - with self.assertRaises(requests.HTTPError): - report.request_with_retry(FakeSession([FakeResponse({}, status_code=404)]), - "GET", "https://x") + client, _ = self.client([FakeResponse({}, status_code=404)]) + with self.assertRaises(APIException): + client.projects.get_project() def test_crowdins_budget_is_ten_attempts_at_sixty_seconds(self): - session = FakeSession([FakeResponse({}, status_code=503)] * 9 + [FakeResponse({"ok": 1})]) + client, session = self.client([FakeResponse({}, status_code=503)] * 9 + + [FakeResponse({"data": {"ok": 1}})]) with NoSleep(): - self.assertEqual(report.request_with_retry(session, "GET", "https://x").json(), {"ok": 1}) + self.assertEqual(client.projects.get_project()["data"], {"ok": 1}) self.assertEqual(len(session.calls), 10) self.assertEqual({kw["timeout"] for _, _, kw in session.calls}, {60}) + def test_fetch_all_follows_full_pages_and_unwraps_the_envelopes(self): + full = {"data": [{"data": {"id": n}} for n in range(500)]} + client, session = self.client([FakeResponse(full), + FakeResponse({"data": [{"data": {"id": 500}}]})]) + items = crowdin_sdk.fetch_all(client.source_strings, "list_strings") + self.assertEqual(len(items), 501) + self.assertEqual(items[-1], {"id": 500}) + self.assertEqual([kw["params"]["offset"] for _, _, kw in session.calls], [0, 500]) + class TestPostToDiscord(unittest.TestCase): def post(self, responses, messages): diff --git a/github_prs/digest.py b/github_prs/digest.py index 78a0068..3337bb3 100755 --- a/github_prs/digest.py +++ b/github_prs/digest.py @@ -42,18 +42,20 @@ import os import sys from datetime import datetime, timedelta, timezone -from operator import itemgetter +from itertools import islice +from operator import attrgetter import requests +from github import Auth, Github +from github.GithubException import GithubException +from github.GithubRetry import GithubRetry sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) from shared import discord, state as dedup # noqa: E402 from shared.discord import MAX_MESSAGE_TEXT_CHARS # noqa: E402 from shared.env import get_env # noqa: E402 -from shared.retry import request_with_retry # noqa: E402 from shared.text import clip, window_label # noqa: E402 -API = "https://api.github.com" DEFAULT_ORG = "session-foundation" # Three days, because the timer runs on weekdays: Monday's window has to reach back # over the weekend. Overlap between consecutive runs is what --state absorbs. @@ -65,6 +67,7 @@ # https://docs.github.com/en/rest/search#about-search SEARCH_RESULT_LIMIT = 1000 PER_PAGE = 100 +ATTEMPTS = 6 MAINTAINERS_FILE = os.path.join(os.path.dirname(os.path.abspath(__file__)), "maintainers.txt") @@ -80,85 +83,55 @@ def load_maintainers(path): return frozenset(logins) -def github_session(token): - session = requests.Session() - session.headers.update({ - "Authorization": f"Bearer {token}", - "Accept": "application/vnd.github+json", - "X-GitHub-Api-Version": "2022-11-28", - }) - return session +def github_client(token): + """Six attempts with backoff on 429 and 5xx, as shared.retry gives every other + service; GithubRetry adds the secondary rate limit, a 403 GitHub only signals in + the body.""" + retry = GithubRetry(total=ATTEMPTS - 1, backoff_factor=1, backoff_max=30, + retry_after_max=60, status_forcelist=[429, *range(500, 600)]) + return Github(auth=Auth.Token(token), per_page=PER_PAGE, timeout=30, retry=retry) -def fetch_json(session, url, **kwargs): - resp = request_with_retry(session, "GET", url, **kwargs) - if resp.status_code >= 400: - sys.exit(f"GitHub {resp.status_code} on {url}: {resp.text[:300]}") - return resp.json() - - -def fetch_repos(session, org): +def fetch_repos(gh, org): """Names of the org's own, live, public repositories. Private repositories are never reported, whatever the token can see: the digest posts to Discord, and nothing about them belongs there. """ - names, page = set(), 1 - while True: - batch = fetch_json(session, f"{API}/orgs/{org}/repos", - params={"per_page": PER_PAGE, "page": page, "type": "all"}) - for repo in batch: - if repo.get("private") or repo.get("fork") or repo.get("archived"): - continue - names.add(repo["name"]) - if len(batch) < PER_PAGE: - return names - page += 1 - - -def search_open_prs(session, org, max_results=SEARCH_RESULT_LIMIT): + return {repo.name for repo in gh.get_organization(org).get_repos(type="all") + if not (repo.private or repo.fork or repo.archived)} + + +def search_open_prs(gh, org, max_results=SEARCH_RESULT_LIMIT): """Every open PR in the org, newest activity first. Returns (items, truncated). `truncated` is the caller's cue that the counts are - a floor rather than a total. + a floor rather than a total. It comes from the item count, not the search's + totalCount: PyGithub reads that off the last-page link, which GitHub caps at + the same 1000, so it can never exceed what was fetched. """ - items, page = [], 1 + results = gh.search_issues(f"org:{org} is:pr is:open", sort="updated", order="desc") limit = min(max_results, SEARCH_RESULT_LIMIT) - while len(items) < limit: - payload = fetch_json(session, f"{API}/search/issues", params={ - "q": f"org:{org} is:pr is:open", - "sort": "updated", - "order": "desc", - "per_page": PER_PAGE, - "page": page, - # The legacy issue-search syntax is gone; without this the endpoint - # rejects the query rather than falling back to it. - "advanced_search": "true", - }) - batch = payload.get("items", []) - items.extend(batch) - total = payload.get("total_count", len(items)) - if len(batch) < PER_PAGE or len(items) >= total: - return items[:limit], total > len(items[:limit]) - page += 1 - return items[:limit], True + items = list(islice(results, limit)) + return items, len(items) >= limit def repo_name(item): """Repo name for a search result — the API gives only the repository's API URL.""" - return item.get("repository_url", "").rsplit("/", 1)[-1] + return item.repository_url.rsplit("/", 1)[-1] def is_bot(item): - return (item.get("user") or {}).get("type") == "Bot" + return item.user.type == "Bot" def author(item): - return (item.get("user") or {}).get("login") or "?" + return item.user.login or "?" -def parse_time(value): - return datetime.fromisoformat(value) +def timestamp(when): + """GitHub's own form, which is what the state file already holds.""" + return when.strftime("%Y-%m-%dT%H:%M:%SZ") def contributor_prs(items, repos, maintainers): @@ -176,11 +149,11 @@ def contributor_prs(items, repos, maintainers): def in_window(prs, cutoff): """The PRs that have moved at all since the cutoff. A PR nobody has touched in three days is not news, whatever its state here.""" - return [pr for pr in prs if parse_time(pr["updated_at"]) >= cutoff] + return [pr for pr in prs if pr.updated_at >= cutoff] def pr_id(pr): - return str(pr.get("id")) + return str(pr.id) def activity_key(pr): @@ -192,7 +165,7 @@ def activity_key(pr): search result and cost a request per PR that moved; this is the deliberate cheaper half of that trade. """ - return pr.get("updated_at") + return timestamp(pr.updated_at) # ---- Dedup state ----------------------------------------------------------- @@ -200,7 +173,7 @@ def activity_key(pr): # The repo#number is never read back. The file is the first thing anyone opens when # the digest reports the wrong thing, and an id alone identifies nothing. STATE = dedup.Tracker(STATE_VERSION, "PR", pr_id, activity_key, "updated_at", - describe=lambda pr: {"pr": f"{repo_name(pr)}#{pr.get('number')}"}) + describe=lambda pr: {"pr": f"{repo_name(pr)}#{pr.number}"}) DEFAULT_RETENTION_DAYS = STATE.retention_days @@ -232,13 +205,13 @@ def age(then, now): def build_pr_line(pr, now, is_new): marker = NEW_MARKER if is_new else UPDATED_MARKER - stamp = age(parse_time(pr["created_at"] if is_new else pr["updated_at"]), now) - title = clip(pr.get("title"), TITLE_CHARS) - if pr.get("draft"): + stamp = age(pr.created_at if is_new else pr.updated_at, now) + title = clip(pr.title, TITLE_CHARS) + if pr.draft: title = f"[draft] {title}" - comments = pr.get("comments") or 0 + comments = pr.comments or 0 replies = f" · 💬{comments}" if comments else "" - return (f"{marker} [#{pr['number']}]({pr['html_url']}) @{author(pr)} · " + return (f"{marker} [#{pr.number}]({pr.html_url}) @{author(pr)} · " f"{stamp}{replies} · {title}") @@ -274,7 +247,7 @@ def group_by_repo(new, updated, now, max_chars=MAX_MESSAGE_TEXT_CHARS): for prs, field, is_new in ((new, "created_at", True), (updated, "updated_at", False)): group = sorted((pr for pr in prs if repo_name(pr) == repo), - key=itemgetter(field), reverse=True) + key=attrgetter(field), reverse=True) entries += [(build_pr_line(pr, now, is_new), pr_id(pr)) for pr in group] repos.append((split_block(f"**{repo}**", entries, max_chars), len(entries))) repos.sort(key=lambda item: -item[1]) @@ -333,9 +306,14 @@ def main(): required=not args.dry_run) maintainers = load_maintainers(args.maintainers) - session = github_session(token) - repos = fetch_repos(session, org) - items, truncated = search_open_prs(session, org) + gh = github_client(token) + try: + repos = fetch_repos(gh, org) + items, truncated = search_open_prs(gh, org) + except GithubException as exc: + sys.exit(f"GitHub {exc.status}: {str(exc.data)[:300]}") + except requests.RequestException as exc: + sys.exit(f"GitHub unreachable: {exc}") prs = contributor_prs(items, repos, maintainers) now = datetime.now(timezone.utc) diff --git a/github_prs/requirements.txt b/github_prs/requirements.txt index 70429a6..3e75fe2 100644 --- a/github_prs/requirements.txt +++ b/github_prs/requirements.txt @@ -1,2 +1,3 @@ # What the deployment runs. The suite is stdlib unittest and imports nothing else. requests==2.34.2 +PyGithub==2.10.0 diff --git a/github_prs/test_digest.py b/github_prs/test_digest.py index 5e9043d..a553386 100644 --- a/github_prs/test_digest.py +++ b/github_prs/test_digest.py @@ -11,16 +11,22 @@ from datetime import datetime, timedelta, timezone from unittest import mock +from github.Issue import IssueSearchResult +from github.Requester import Requester + sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) import digest # noqa: E402 from shared import discord # noqa: E402 +from shared.testing import NoSleep # noqa: E402 NOW = datetime(2026, 9, 24, 12, 0, tzinfo=timezone.utc) CUTOFF = NOW - timedelta(hours=25) +GH = digest.github_client("token") -def pr(number=1, login="octocat", repo="session-android", created="2026-09-24T08:00:00Z", - updated=None, title="Fix a thing", user_type="User", **extra): +def pr_item(number=1, login="octocat", repo="session-android", created="2026-09-24T08:00:00Z", + updated=None, title="Fix a thing", user_type="User", **extra): + """A search result as the API returns one.""" item = { "id": 10_000 + number, "number": number, @@ -30,11 +36,18 @@ def pr(number=1, login="octocat", repo="session-android", created="2026-09-24T08 "user": {"login": login, "type": user_type}, "created_at": created, "updated_at": updated or created, + "draft": False, + "comments": 0, } item.update(extra) return item +def pr(*args, **kwargs): + """A search result as PyGithub hands it to the digest.""" + return IssueSearchResult(GH.requester, {}, pr_item(*args, **kwargs), completed=True) + + class TestMaintainers(unittest.TestCase): def test_comments_and_blanks_are_ignored(self): with mock.patch("builtins.open", mock.mock_open(read_data= @@ -73,8 +86,7 @@ class TestWindow(unittest.TestCase): def test_only_prs_that_moved_since_the_cutoff_are_considered(self): fresh = pr(1, updated="2026-09-24T09:00:00Z") stale = pr(2, created="2026-08-01T08:00:00Z", updated="2026-08-02T08:00:00Z") - self.assertEqual([p["number"] for p in digest.in_window([fresh, stale], CUTOFF)], - [1]) + self.assertEqual([p.number for p in digest.in_window([fresh, stale], CUTOFF)], [1]) def test_the_cutoff_itself_counts_as_inside_the_window(self): edge = pr(1, updated=CUTOFF.strftime("%Y-%m-%dT%H:%M:%SZ")) @@ -94,21 +106,21 @@ def state(self, *records): def test_a_pr_never_reported_is_new(self): new, changed, unchanged = digest.STATE.partition([pr(1)], digest.STATE.empty()) - self.assertEqual([p["number"] for p in new], [1]) + self.assertEqual([p.number for p in new], [1]) self.assertEqual((changed, unchanged), ([], [])) def test_a_pr_that_moved_since_it_was_reported_is_changed(self): item = pr(1, updated="2026-09-24T09:00:00Z") new, changed, unchanged = digest.STATE.partition( [item], self.state((digest.pr_id(item), "2026-09-20T09:00:00Z"))) - self.assertEqual([p["number"] for p in changed], [1]) + self.assertEqual([p.number for p in changed], [1]) self.assertEqual((new, unchanged), ([], [])) def test_a_pr_that_has_not_moved_is_dropped(self): item = pr(1, updated="2026-09-24T09:00:00Z") new, changed, unchanged = digest.STATE.partition( [item], self.state((digest.pr_id(item), "2026-09-24T09:00:00Z"))) - self.assertEqual([p["number"] for p in unchanged], [1]) + self.assertEqual([p.number for p in unchanged], [1]) self.assertEqual((new, changed), ([], [])) @@ -300,38 +312,91 @@ def test_only_the_first_message_carries_the_header(self): self.assertLess(len(rendered), discord.MAX_MESSAGE_TEXT_CHARS * 2) +class FakeHttpResponse: + def __init__(self, status, headers, payload): + self.status, self.headers, self.payload = status, headers, payload + + def getheaders(self): + return self.headers.items() + + def read(self): + return json.dumps(self.payload) + + +class FakeConnection: + """PyGithub's connection interface, answering from a queue and recording each request.""" + queue, calls = [], [] + + def __init__(self, *args, **kwargs): + pass + + def request(self, verb, url, input, headers, stream=False): + self.calls.append((verb, url)) + + def getresponse(self): + return FakeHttpResponse(*self.queue.pop(0)) + + def close(self): + pass + + +def page(payload, next_page=None): + headers = {"content-type": "application/json"} + if next_page is not None: + headers["link"] = f'; rel="next"' + return 200, headers, payload + + +def repo(name, **flags): + return {"name": name, "fork": False, "archived": False, "private": False, **flags} + + +ORG = {"login": "org", "url": "https://api.github.com/orgs/org"} + + class TestFetching(unittest.TestCase): + def setUp(self): + FakeConnection.queue.clear() + FakeConnection.calls.clear() + Requester.injectConnectionClasses(FakeConnection, FakeConnection) + self.addCleanup(Requester.resetConnectionClasses) + + def fetch(self, *responses, call): + FakeConnection.queue.extend(responses) + with NoSleep(): + return call(digest.github_client("token"), "org") + def test_forks_archived_and_private_repos_are_left_out(self): - page = [{"name": "session-android", "fork": False, "archived": False}, - {"name": "session-pysogs", "fork": True, "archived": False}, - {"name": "retired", "fork": False, "archived": True}, - {"name": "internal", "fork": False, "archived": False, "private": True}] - with mock.patch.object(digest, "fetch_json", return_value=page): - self.assertEqual(digest.fetch_repos(None, "org"), {"session-android"}) + listing = [repo("session-android"), repo("session-pysogs", fork=True), + repo("retired", archived=True), repo("internal", private=True)] + self.assertEqual(self.fetch(page(ORG), page(listing), call=digest.fetch_repos), + {"session-android"}) def test_repo_pagination_follows_full_pages(self): - pages = [[{"name": f"r{n}", "fork": False, "archived": False} - for n in range(digest.PER_PAGE)], - [{"name": "last", "fork": False, "archived": False}]] - with mock.patch.object(digest, "fetch_json", side_effect=pages): - self.assertEqual(len(digest.fetch_repos(None, "org")), digest.PER_PAGE + 1) + first = [repo(f"r{n}") for n in range(digest.PER_PAGE)] + names = self.fetch(page(ORG), page(first, next_page=2), page([repo("last")]), + call=digest.fetch_repos) + self.assertEqual(len(names), digest.PER_PAGE + 1) + self.assertEqual(len(FakeConnection.calls), 3) def test_search_stops_when_the_results_run_out(self): - payload = {"total_count": 2, "items": [pr(1), pr(2)]} - with mock.patch.object(digest, "fetch_json", return_value=payload) as fetch: - items, truncated = digest.search_open_prs(None, "org") - self.assertEqual(len(items), 2) + payload = {"total_count": 2, "incomplete_results": False, + "items": [pr_item(1), pr_item(2)]} + items, truncated = self.fetch(page(payload), call=digest.search_open_prs) + self.assertEqual([p.number for p in items], [1, 2]) self.assertFalse(truncated) - self.assertEqual(fetch.call_count, 1) + self.assertEqual(len(FakeConnection.calls), 1) def test_search_stops_at_the_result_ceiling_rather_than_erroring(self): - full = {"total_count": 1500, - "items": [pr(n) for n in range(digest.PER_PAGE)]} - with mock.patch.object(digest, "fetch_json", return_value=full) as fetch: - items, truncated = digest.search_open_prs(None, "org") + pages = digest.SEARCH_RESULT_LIMIT // digest.PER_PAGE + full = [page({"total_count": 1500, "incomplete_results": False, + "items": [pr_item(n) for n in range(digest.PER_PAGE)]}, + next_page=n + 2) + for n in range(pages)] + items, truncated = self.fetch(*full, call=digest.search_open_prs) self.assertEqual(len(items), digest.SEARCH_RESULT_LIMIT) self.assertTrue(truncated) - self.assertEqual(fetch.call_count, digest.SEARCH_RESULT_LIMIT // digest.PER_PAGE) + self.assertEqual(len(FakeConnection.calls), pages) if __name__ == "__main__": diff --git a/shared/testing.py b/shared/testing.py index da98e4b..dba636a 100644 --- a/shared/testing.py +++ b/shared/testing.py @@ -18,6 +18,10 @@ def __init__(self, payload, status_code=200, retry_after="0"): def json(self): return self._payload + @property + def content(self): + return self.text.encode() + def raise_for_status(self): if self.status_code >= 400: raise requests.HTTPError(f"{self.status_code}", response=self)