Jens-G commented on PR #3521: URL: https://github.com/apache/thrift/pull/3521#issuecomment-4526744235
### Code review Found 5 issues: 1. **6 commits instead of 1 squashed commit** ([AGENTS.md](https://github.com/apache/thrift/blob/f8246e5717e6c33644d48d77e027c0cc2bcdc7ad/AGENTS.md#L48-L50) says "One commit per issue (squash before submitting)" and the §6 checklist requires a "Single squashed commit") 2. **`Client: Compiler (general)` silently falls through to "(No Section)"** — `CLIENT_SECTION_MAP` only contains the key `"compiler"`, but historical commits use the form `Compiler (general)` which after `.lower()` becomes `"compiler (general)"` — not present in the map. 16 commits in the repo history use this exact form. https://github.com/apache/thrift/blob/f8246e5717e6c33644d48d77e027c0cc2bcdc7ad/build/gen-changes.py#L135-L161 3. **Unauthenticated rate-limit delay is 40× too short** — the code comment says unauthenticated GitHub requests are limited to 60 req/hr, but `delay = 1.5` s/request permits ~2,400 req/hr. Staying within the limit requires 60 s/request. Any unauthenticated run against a repo with more than ~60 PRs will exhaust the limit and silently lose label data. https://github.com/apache/thrift/blob/f8246e5717e6c33644d48d77e027c0cc2bcdc7ad/build/gen-changes.py#L455-L459 4. **`CLIENT_SECTION_MAP` is missing many historically-used `Client:` trailer values** — values such as `c#` (73 commits in history), `python` (60), `erlang` (47), `ruby` (27), `hx` (Haxe, 13), `csharp` (13), `hs` (Haskell, 12), `c++` (4), `compiler` (6 — covered, but `compiler (general)` is not) are absent. These silently fall to "(No Section)". By contrast, `GITHUB_LABEL_MAP` does include `c#`, `c++`, `erlang`, `ruby`, etc. https://github.com/apache/thrift/blob/f8246e5717e6c33644d48d77e027c0cc2bcdc7ad/build/gen-changes.py#L135-L161 5. **`--no-commits` help text understates the flag's effect** — the help string says "exclude ticket-less commits from output," but the implementation excludes all commit entries, including commits that reference a valid THRIFT ticket that simply was not found in the JIRA query (wrong fixVersion, not yet resolved, etc.). The inline comment at line 699 correctly describes this broader behavior, but the user-facing help text does not. https://github.com/apache/thrift/blob/f8246e5717e6c33644d48d77e027c0cc2bcdc7ad/build/gen-changes.py#L791-L795 🤖 Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
