justinmclean commented on PR #4020:
URL: https://github.com/apache/iggy/pull/4020#issuecomment-5503290197
The three methods and the wrapper types look right and the tests are well
shaped. One regression in the stub, plus two small points.
### `ConsumerGroupMember` lost its `@typing.final`
(`foreign/python/apache_iggy.pyi:464`)
The new `ConsumerGroupInfo` block was inserted between the existing
`@typing.final` line and `class ConsumerGroupMember:`, so the decorator now
applies to `ConsumerGroupInfo` and `ConsumerGroupMember` is left undecorated:
```text
@typing.final
class ConsumerGroupInfo:
...
The unique identifier (numeric) of the consumer group.
"""
class ConsumerGroupMember: # <- was @typing.final on the base commit
```
Nothing in this PR touches `ConsumerGroupMember`, so this is an edit the
generator would not produce.
`.github/actions/python-maturin/pre-merge/action.yml:108` runs the generator
and compares the result to the tracked file, so please run exactly that from
`foreign/python` and commit whatever it writes, rather than editing the stub by
hand:
```bash
cd foreign/python
cargo run --bin stub_gen
uv run --no-sync ruff format apache_iggy.pyi
uv run --no-sync ruff check --fix apache_iggy.pyi
git diff --exit-code -- apache_iggy.pyi
```
The rest of the stub, including the placement of `get_me` / `get_client` /
`get_clients` in source order, already matches what the generator emits.
### New direct `iggy_common` dependency (`foreign/python/Cargo.toml:41`,
`foreign/python/src/client_info.rs:18`)
This is the first direct `iggy_common` dependency in the Python binding;
every other module reaches its Rust types through `iggy::prelude`. The module
splits its imports across both, because `core/sdk/src/prelude.rs:50` re-exports
`ClientInfoDetails` but not `ClientInfo` or `ConsumerGroupInfo`.
It works. Worth a maintainer's call whether the better fix is adding those
two types to the `core/sdk` prelude, which would keep the binding on one
dependency.
### Smaller observations
- `foreign/python/src/client_info.rs:30` - the `From<RustConsumerGroupInfo>
for ConsumerGroupInfo` impl is never called. The `consumer_groups` getter at
line 153 cannot use it: it holds `&self`, and `iggy_common::ConsumerGroupInfo`
derives only `Debug, Serialize, Deserialize`
(`core/common/src/types/client/client_info.rs:70`), so there is nothing to move
or clone from. Changing the impl to `From<&RustConsumerGroupInfo>` would make
it usable and let the getter read `.map(ConsumerGroupInfo::from)`. Dropping the
impl works too.
---
This review was drafted by an AI-assisted tool (Apache Magpie) so may
contain mistakes. If you think one of them is misapplied, please reply on the
PR, and a maintainer will weigh in.
--
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]
