Jens-G commented on PR #3753: URL: https://github.com/apache/thrift/pull/3753#issuecomment-5635392746
### Code review No blocking issues found. Checked for bugs and CLAUDE.md compliance. A few suggestions, below the bar for an issue but verified: - A small formality for when you next push: CONTRIBUTING.md asks for a colon after the ticket id in the commit subject (`THRIFT-4623: Fix python complex nested struct includes`, the PR title can follow suit) and a `Client: py` line below it. https://github.com/apache/thrift/blob/bb4bf37da9c565e5e5d473d6ffb3562817541a07/CONTRIBUTING.md#L18-L27 - `type` now goes through `get_true_type()`, and the same variable feeds the unchanged immutable branch below it. With `-gen py:enum`, a field typed as a typedef of an enum, in an exception or a `python.immutable` struct, now gets the enum coercion (`x if hasattr(x, 'value') else Color.__members__.get(x)`) instead of a plain assignment, so an int passed there is stored as `None`. Non-typedef enum fields already behave that way, so this may be acceptable, but it is a behaviour change outside the struct-default fix. A separate variable for the new check would avoid it. https://github.com/apache/thrift/blob/bb4bf37da9c565e5e5d473d6ffb3562817541a07/compiler/cpp/src/thrift/generate/t_py_generator.cc#L910-L926 - With `-gen py:type_hints`, the new default produces signatures like `nested: typing.Optional[C] = _THRIFT_DEFAULT`. A type checker rejects that, because `object()` is neither `C` nor `None`. Runtime behaviour is not affected. https://github.com/apache/thrift/blob/bb4bf37da9c565e5e5d473d6ffb3562817541a07/compiler/cpp/src/thrift/generate/t_py_generator.cc#L2786-L2794 🤖 Generated with [Claude Code](https://claude.ai/code) -- 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]
