AbsaOSS / AbsaOSS/generate-release-notes
Tech-debt: Since-time mode: `since=None` can crash `get_issues()`, and API failures silently degrade to empty results
- Lingua principale
- Python
- Stelle
- 13
- Fork
- 0
- Merge medio
- 6h 50m
- PR unite (30g)
- 7
Descrizione
Found while reviewing PR #337 (Copilot comments on the pagination-safety refactor, since reverted from that PR as out of scope for #335).
## Problem 1 — `since=None` can raise inside PyGithub
In `DataMiner._get_issues`, `data.since` is `Optional[datetime]` and gets passed straight through to `Repository.get_issues(..., since=data.since)`. PyGithub's implementation asserts `is_optional(since, datetime)`, i.e. `isinstance(v, NotSetType) or isinstance(v, datetime)` — explicitly passing `None` satisfies neither, so `get_issues(since=None)` raises `AssertionError: None` instead of falling back to its default. Today this is masked because the call goes through `_safe_call`'s untyped wrapper, and is *also* practically unreachable in this exact spot (a GitHub release always has `created_at`) — but it's a latent crash risk, and the same pattern (attribute typed `Optional[datetime]`, passed unconditionally as a kwarg) could easily recur elsewhere.
Fix: only pass `since` when it's not `None` (omit the kwarg so PyGithub's `NotSet` default applies), instead of suppressing the type checker.
## Problem 2 — `_safe_call` failures silently produce incomplete results in since-time mode
`_safe_call` catches `GithubException`/network errors and returns `None`. In compare mode, a `None` result is treated as fatal (`sys.exit(1)`) — e.g. `comparison is None`. In since-time mode, several fetches (`repo.get_pulls`, `repo.get_commits`, `repo.get_releases`, `repo.get_issues`) instead do `... or []`, so a transient API failure silently becomes "0 PRs found" / "0 commits found" / "treat this as the first release" and the run completes "successfully" with incomplete or wrong release notes.
Decide and apply consistently: should a `_safe_call` failure during since-time mining be fatal (matching compare mode), or is partial/degraded output acceptable with a loud warning? Whichever is chosen, apply it uniformly across all the since-time-mode fetches instead of the current ad hoc mix of "crash" (pre-existing, for some paths) and "silently continue" (introduced in #337 for others).
Update - see latest [post from copilot review](https://github.com/AbsaOSS/generate-release-notes/pull/337#pullrequestreview-5007123333)
Guida per i contributori
Apri la guida per i contributori
Valutazione
Questa issue non è ancora stata valutata.