Self-Review: PR #106
Self-review against ae94ba7be6c4dbf598ed3bf4766ee0d0a1a9c3be
Phase 1: Independent Findings
Addressed all three MINOR findings from review #3152 in commit ae94ba7:
Re: Comment #18889 (type assertion in supersedeOldReviews):
Added a doc comment explaining that the assertion is…
Fixed in ae94ba7. Added a doc comment explaining that the type assertion is guaranteed to succeed given the caller's provider switch (only "gitea" reaches this point), and that the !ok branch guards against future refactors like wrapping the adapter in a decorator.
Acknowledged. The current structure reads top-to-bottom: scan forward for a resolution, then differentiate the error. Moving the check before the loop would require storing the message in a variable or duplicating the scan logic. The linear post-loop flow is clearer. Keeping as-is.
Addressing review NITs — pushing back on all three as the code structure is intentional and the missing test already exists.
This edge case IS tested — see TestTranslate_HunkHeaderAtEnd (line 361), which tests exactly the scenario where a hunk-header is the last position with no subsequent new-file line. That test covers the error path the NIT mentions.
Same reasoning as the earlier NIT on this topic: the post-loop position is the natural place for error differentiation. Computing it before the loop would add a variable that's only used in the failure path and separate the error logic from where the failure is detected. Current flow is: attempt resolution → explain failure. Keeping as-is.
Self-Review: PR #106
Self-review against 48658ca6a49578e14e260e4c09ddfbf9b51a21e8
Phase 1: Independent Findings
None — diff looks clean. All previous issues have been addressed in this…
Addressed all findings from self-review at 94cf894c in commit 48658ca:
Finding #1 (MINOR) — findOwnReview dead code: ✅ Removed the function and its test (TestFindOwnReview). `findAllOwnR…
Self-Review: PR #106
Self-review against 94cf894c49e0e8851a484f491fba30202edb72f2
Phase 1: Independent Findings
No change needed — vcs.ReviewEvent is defined as type ReviewEvent string (see vcs/types.go). The %s verb is correct for string-backed types; Go's fmt package uses the underlying string…
Pushing back: The --base-url is operator-controlled configuration (env var or CLI flag), not user input. The client already enforces HTTPS-only (rejecting non-TLS URLs) and strips Authorization…
Fixed in 94cf894 — added an explicit ORDERING comment noting the dependency on registration-before-parse evaluation order.
Fixed in 94cf894 — removed the redundant explicit case vcs.ReviewEventComment. The default case handles it.
Acknowledged — the type assertion is intentionally provider-specific. supersedeOldReviews is a provider-dispatched function by design (the switch on provider at the top). GitHub uses…
Fixed in 94cf894 — doJSONRequest now has the same 429 retry logic as doRequest, including Retry-After header support and configurable backoff. Added tests for both retry-success and…