justinmclean commented on PR #4017:
URL: https://github.com/apache/iggy/pull/4017#issuecomment-5503304595
Clean, minimal binding. It follows the existing `purge_topic` shape exactly
and does everything #4014 asked for: both methods on the `#[pymethods]` block,
regenerated stubs, and a test that round-trips the count through `get_topic`.
Two observations, neither blocking.
### Smaller observations
- `foreign/python/src/client.rs:791` and `:824` — the `Raises:` block says
`RuntimeError: If an identifier is invalid or the request fails.`, but an
invalid identifier never reaches the request. `impl TryFrom<&PyIdentifier> for
Identifier` in `foreign/python/src/identifier.rs` maps both the string and the
numeric arm to `PyValueError`, and `TryFrom<PyIdentifier>` delegates to it, so
`create_partitions("", "topic", 2)` raises `ValueError`.
`create_consumer_group`, the next method in this file, documents the same code
path correctly:
```text
/// Raises:
/// ValueError: If an identifier is invalid.
/// RuntimeError: If the request fails.
```
Splitting the line the same way and re-running `cargo run --bin stub_gen`
would keep `apache_iggy.pyi` in step. To be clear about where this came from:
it is a file-wide pattern, not something this PR introduced. `purge_topic`,
`delete_topic` (`:731`) and `get_topics` (`:628`) conflate the two the same
way, and `get_topic` collapses it to a single "raises RuntimeError on failure"
line. Fixing the whole file is its own change; matching `create_consumer_group`
in the two new methods is enough here.
- `foreign/python/tests/test_partition.py:33-41` — this matches #4014's test
spec exactly, so nothing is missing. One optional strengthening if you feel
like it: the count assertions hold whichever partitions the server removed, and
#4014's design note is specifically that `delete_partitions` drops the *last*
N. `TopicDetails.partitions[].id` is exposed and
`test_topic.py::TestGetTopic::test_get_topic_partitions` already reads ids that
way, so asserting that the surviving ids are the first N would pin the
documented semantics rather than just the arithmetic.
Nothing here blocks the merge.
---
This review was drafted by an AI-assisted tool (Apache Magpie), so it 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]
