Adding data (aka json) to the py_lib#1318
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change adds raw track- and group-level publishing and consumption across the Rust FFI and Python wrapper. New UniFFI types MoqTrackProducer, MoqGroupProducer, MoqTrackConsumer, and MoqGroupConsumer were introduced; MoqBroadcastProducer gained publish_track and consume methods; MoqBroadcastConsumer gained subscribe_track. The Python package now exposes GroupProducer, TrackProducer, GroupConsumer, TrackConsumer, extends BroadcastProducer/BroadcastConsumer with corresponding helpers, adds async iterators for groups/frames, new tests, and increments the package version to 0.0.3. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
rs/moq-ffi/src/consumer.rs (1)
125-137: Empty/extra-frame groups are silently dropped — confirm this is intended.
RawInner::nextonly returns the first frame of each group; if a group contains zero frames it's skipped withcontinue, and if it contains more than one frame the additional frames are discarded (the next call gets a fresh group). This matches the producer (one-frame-per-group), but if a remote peer ever publishes raw tracks differently (e.g., multiple frames per group, as moq-boy may do), receivers will silently lose data.Consider either:
- documenting the one-frame-per-group contract on
subscribe_raw, or- draining all frames in the current group before advancing to the next group.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-ffi/src/consumer.rs` around lines 125 - 137, RawInner::next currently returns only the first frame from a group and discards any additional frames (skipping empty groups), which can silently drop data; update RawInner::next so that after retrieving the first frame from a group (via group.read_frame().await?), you drain the rest of the frames in that same group before returning (e.g., loop calling group.read_frame().await? until it yields None) so no frames from the group are silently left behind, keeping references to RawInner::next, track.next_group(), and group.read_frame() to locate the change.py/moq-lite/moq_lite/subscribe.py (1)
41-45: Optional:bytes(frame)is likely redundant.
MoqRawConsumer.next()returnsVec<u8>which UniFFI already maps to Pythonbytes, sobytes(frame)performs an unnecessary copy. You canreturn framedirectly (matching howMediaConsumer.__anext__returns the frame as-is). Not a correctness issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@py/moq-lite/moq_lite/subscribe.py` around lines 41 - 45, In __anext__ of the subscriber (async def __anext__(self) -> bytes) avoid the redundant bytes(frame) copy: since MoqRawConsumer.next() already returns a bytes-like object (UniFFI maps Vec<u8> to Python bytes), simply return frame directly; update the return statement in __anext__ to return frame (mirroring MediaConsumer.__anext__) and remove the extra conversion to eliminate the unnecessary allocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-ffi/src/producer.rs`:
- Around line 109-116: The write_frame implementation for MoqRawProducer lacks
the runtime enter guard and can panic when calling TrackProducer::append_group
(which uses tokio::time::Instant); add the same RUNTIME.enter() guard used in
publish_raw/finish/MoqMediaProducer::write_frame (e.g., let _rt =
RUNTIME.enter(); at the start of pub fn write_frame(&self, ...) ) so the tokio
reactor is active for calls like track.append_group(), then proceed with locking
self.inner and the existing append_group/write_frame/finish sequence.
---
Nitpick comments:
In `@py/moq-lite/moq_lite/subscribe.py`:
- Around line 41-45: In __anext__ of the subscriber (async def __anext__(self)
-> bytes) avoid the redundant bytes(frame) copy: since MoqRawConsumer.next()
already returns a bytes-like object (UniFFI maps Vec<u8> to Python bytes),
simply return frame directly; update the return statement in __anext__ to return
frame (mirroring MediaConsumer.__anext__) and remove the extra conversion to
eliminate the unnecessary allocation.
In `@rs/moq-ffi/src/consumer.rs`:
- Around line 125-137: RawInner::next currently returns only the first frame
from a group and discards any additional frames (skipping empty groups), which
can silently drop data; update RawInner::next so that after retrieving the first
frame from a group (via group.read_frame().await?), you drain the rest of the
frames in that same group before returning (e.g., loop calling
group.read_frame().await? until it yields None) so no frames from the group are
silently left behind, keeping references to RawInner::next, track.next_group(),
and group.read_frame() to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6ec4042-f766-4728-9022-d80ae3d675b2
📒 Files selected for processing (8)
demo/justfilepy/moq-lite/moq_lite/__init__.pypy/moq-lite/moq_lite/publish.pypy/moq-lite/moq_lite/subscribe.pypy/moq-lite/pyproject.tomlpy/moq-lite/tests/test_local.pyrs/moq-ffi/src/consumer.rsrs/moq-ffi/src/producer.rs
|
@Qizot 👀 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-dummy-raw/src/main.rs`:
- Around line 43-45: The CLI field rate (struct field rate parsed by clap) can
be zero or negative which makes the downstream expression
Duration::from_secs_f64(1.0 / cli.rate) produce non-finite/negative input and
panic; validate or constrain rate immediately after parsing (or via clap
validator) to ensure cli.rate > 0.0 and return a user-friendly error for
non-positive values so Duration::from_secs_f64 is only called with a finite
positive value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2db43256-5fb9-442b-a2c7-f6dcf9254327
⛔ Files ignored due to path filters (10)
Cargo.lockis excluded by!**/*.lockdemo/relay/heaptrack.moq-relay.75889.zstis excluded by!**/*.zstdemo/relay/heaptrack.moq-relay.76064.zstis excluded by!**/*.zstdemo/relay/heaptrack.moq-relay.76598.zstis excluded by!**/*.zstdemo/relay/heaptrack.moq-relay.79333.zstis excluded by!**/*.zstdemo/relay/heaptrack.moq-relay.82171.zstis excluded by!**/*.zstdemo/relay/heaptrack.moq-relay.86157.zstis excluded by!**/*.zstheaptrack.just.36493.zstis excluded by!**/*.zstheaptrack.just.49492.zstis excluded by!**/*.zstuv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlrs/moq-dummy-raw/Cargo.tomlrs/moq-dummy-raw/src/main.rs
✅ Files skipped from review due to trivial changes (2)
- Cargo.toml
- rs/moq-dummy-raw/Cargo.toml
| /// Frames per second. | ||
| #[arg(long, default_value_t = 20.0)] | ||
| rate: f64, |
There was a problem hiding this comment.
Validate rate to avoid panic on zero/negative input.
clap accepts any f64, so --rate 0 or a negative value flows into line 75: Duration::from_secs_f64(1.0 / cli.rate). 1.0 / 0.0 yields inf, and Duration::from_secs_f64 panics on non-finite or negative values, killing the binary with an unhelpful message. Consider constraining the parsed range or rejecting invalid values up front.
🛡️ Proposed fix
/// Frames per second.
- #[arg(long, default_value_t = 20.0)]
+ #[arg(long, default_value_t = 20.0, value_parser = parse_positive_rate)]
rate: f64,
}
+
+fn parse_positive_rate(s: &str) -> Result<f64, String> {
+ let v: f64 = s.parse().map_err(|e: std::num::ParseFloatError| e.to_string())?;
+ if v.is_finite() && v > 0.0 {
+ Ok(v)
+ } else {
+ Err(format!("rate must be a positive finite number, got {v}"))
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Frames per second. | |
| #[arg(long, default_value_t = 20.0)] | |
| rate: f64, | |
| /// Frames per second. | |
| #[arg(long, default_value_t = 20.0, value_parser = parse_positive_rate)] | |
| rate: f64, | |
| } | |
| fn parse_positive_rate(s: &str) -> Result<f64, String> { | |
| let v: f64 = s.parse().map_err(|e: std::num::ParseFloatError| e.to_string())?; | |
| if v.is_finite() && v > 0.0 { | |
| Ok(v) | |
| } else { | |
| Err(format!("rate must be a positive finite number, got {v}")) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-dummy-raw/src/main.rs` around lines 43 - 45, The CLI field rate
(struct field rate parsed by clap) can be zero or negative which makes the
downstream expression Duration::from_secs_f64(1.0 / cli.rate) produce
non-finite/negative input and panic; validate or constrain rate immediately
after parsing (or via clap validator) to ensure cli.rate > 0.0 and return a
user-friendly error for non-positive values so Duration::from_secs_f64 is only
called with a finite positive value.
Restructures the raw track FFI to mirror moq-lite's GroupProducer / GroupConsumer pattern: callers get a group handle from append_group() / next_group() and then write_frame() / read_frame() on it. The flat frame iterator is gone — each group is addressable, sequence-numbered, and independently consumable. - Add MoqRawGroupProducer and MoqRawGroupConsumer, each exposing sequence() - Add consume() on MoqBroadcastProducer / MoqRawProducer / MoqRawGroupProducer for direct local pub/sub - Add RUNTIME.enter() guard to MoqRawProducer::write_frame for consistency - Drop the redundant bytes(frame) cast in the Python consumer - Expand Python test suite to 24 tests covering sequences, parallel groups, multi-frame groups, empty payloads, error paths, and the new direct-consume flows - Remove the moq-dummy-raw debug crate, committed heaptrack .zst profiles, and the unrelated demo/justfile serve recipe Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@py/moq-lite/tests/test_local.py`:
- Around line 282-305: Replace the broad pytest.raises(Exception) assertions in
tests test_raw_group_finish_twice_fails and
test_raw_track_write_after_finish_fails with a narrow check for the concrete FFI
error (or at minimum a message match); specifically, when calling
group.finish(), track.finish(), group.write_frame(b"too late"),
track.write_frame(b"late") and track.append_group() change
pytest.raises(Exception) to pytest.raises(moq.MoqError) (or the actual exception
class raised by the Rust FFI) or use pytest.raises(<ExceptionClass>,
match="closed|finished") so the tests assert the expected closed/finished error
from BroadcastProducer.publish_raw, append_group, finish, and write_frame.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23bfc49b-713c-42ed-a126-3dae9fd1c517
📒 Files selected for processing (7)
py/moq-lite/moq_lite/__init__.pypy/moq-lite/moq_lite/publish.pypy/moq-lite/moq_lite/subscribe.pypy/moq-lite/tests/test_local.pyrs/moq-ffi/src/consumer.rsrs/moq-ffi/src/origin.rsrs/moq-ffi/src/producer.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- py/moq-lite/moq_lite/init.py
- rs/moq-ffi/src/consumer.rs
- rs/moq-ffi/src/producer.rs
| with pytest.raises(Exception): | ||
| group.write_frame(b"too late") | ||
|
|
||
|
|
||
| def test_raw_group_finish_twice_fails(): | ||
| broadcast = moq.BroadcastProducer() | ||
| track = broadcast.publish_raw("t") | ||
| group = track.append_group() | ||
| group.finish() | ||
|
|
||
| with pytest.raises(Exception): | ||
| group.finish() | ||
|
|
||
|
|
||
| def test_raw_track_write_after_finish_fails(): | ||
| broadcast = moq.BroadcastProducer() | ||
| track = broadcast.publish_raw("t") | ||
| track.finish() | ||
|
|
||
| with pytest.raises(Exception): | ||
| track.write_frame(b"late") | ||
|
|
||
| with pytest.raises(Exception): | ||
| track.append_group() |
There was a problem hiding this comment.
Narrow the pytest.raises exception types.
Ruff B017 flags these four pytest.raises(Exception) calls: they will pass on any exception (including unrelated AttributeError/TypeError/import errors), which hides regressions. Prefer the concrete FFI error class (e.g. the MoqError surfaced by the Rust layer, or whatever the FFI raises on closed/finished state) or at minimum match= on the message.
🔧 Suggested tightening (adjust to the actual exception type)
- with pytest.raises(Exception):
+ with pytest.raises(Exception, match=r"(?i)finish|closed|done"):
group.write_frame(b"too late")Applied similarly to lines 292, 301, 304. Same treatment would also benefit the pre-existing test_unknown_format (line 87) and test_finish_closes_producer (line 216), though those are outside this diff.
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 282-282: Do not assert blind exception: Exception
(B017)
[warning] 292-292: Do not assert blind exception: Exception
(B017)
[warning] 301-301: Do not assert blind exception: Exception
(B017)
[warning] 304-304: Do not assert blind exception: Exception
(B017)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@py/moq-lite/tests/test_local.py` around lines 282 - 305, Replace the broad
pytest.raises(Exception) assertions in tests test_raw_group_finish_twice_fails
and test_raw_track_write_after_finish_fails with a narrow check for the concrete
FFI error (or at minimum a message match); specifically, when calling
group.finish(), track.finish(), group.write_frame(b"too late"),
track.write_frame(b"late") and track.append_group() change
pytest.raises(Exception) to pytest.raises(moq.MoqError) (or the actual exception
class raised by the Rust FFI) or use pytest.raises(<ExceptionClass>,
match="closed|finished") so the tests assert the expected closed/finished error
from BroadcastProducer.publish_raw, append_group, finish, and write_frame.
Renames the FFI/Python track API to match moq-lite's TrackProducer / TrackConsumer naming. Since the moq-lite layer is always "arbitrary bytes," the Raw prefix is redundant — media is the specialization, not the other way around. - MoqRawProducer / MoqRawConsumer -> MoqTrackProducer / MoqTrackConsumer - MoqRawGroupProducer / MoqRawGroupConsumer -> MoqGroupProducer / MoqGroupConsumer - Python RawProducer / RawConsumer -> TrackProducer / TrackConsumer - Python RawGroupProducer / RawGroupConsumer -> GroupProducer / GroupConsumer - publish_raw / subscribe_raw -> publish / subscribe Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rs/moq-ffi/src/producer.rs (1)
82-93: Track priority is hardcoded to0.
moq_lite::Track { name, priority: 0 }hardcodes priority, matchingsubscribe_media/subscribeon the consumer side. That's fine as a first cut, but note that once any caller wants non-default prioritization they'll need a breaking signature change here. Consider whether apublish_with_priority(name, priority)overload (or an optionalpriority: Option<u8>) is worth adding now to avoid churn later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-ffi/src/producer.rs` around lines 82 - 93, The publish method currently hardcodes track priority to 0 (moq_lite::Track { name, priority: 0 }), which prevents callers from choosing non-default prioritization; add an overload like publish_with_priority(name: String, priority: u8) (or change publish to accept Option<u8>) so callers can supply a priority, propagate that value into moq_lite::Track when creating the track inside publish/publish_with_priority, and keep the existing publish(name) as a convenience wrapper delegating to the new method to avoid breaking callers; update construction locations where MoqTrackProducer is created so they call the new API.py/moq-lite/tests/test_local.py (1)
231-486: Test names still use the oldraw_prefix.The commits in this PR explicitly dropped the
Rawprefix from the FFI and Python APIs (publish_raw/subscribe_raw→publish/subscribe,RawProducer→TrackProducer, etc.). The new tests, however, retaintest_raw_*names (e.g.test_raw_append_group_sequence_increments,test_raw_publish_consume,test_raw_multi_frame_group, …), which now misrepresents what's being tested — these exercise the regular track API, not a separate "raw" surface. Consider renaming totest_track_*/test_group_*(or similar) for consistency with the final naming.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@py/moq-lite/tests/test_local.py` around lines 231 - 486, Rename tests that still use the old "raw" naming to reflect the new Track/Group API: change function names like test_raw_append_group_sequence_increments → test_track_append_group_sequence_increments, test_raw_publish_consume → test_track_publish_consume, test_raw_multi_frame_group → test_group_multi_frame (or similar), and update all other test functions prefixed with test_raw_ to test_track_ or test_group_ as appropriate so names match the current APIs (refer to symbol names BroadcastProducer.publish, TrackProducer.append_group, and group.consume in the file to locate related tests and ensure names reflect "track" or "group" semantics).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@py/moq-lite/tests/test_local.py`:
- Around line 231-486: Rename tests that still use the old "raw" naming to
reflect the new Track/Group API: change function names like
test_raw_append_group_sequence_increments →
test_track_append_group_sequence_increments, test_raw_publish_consume →
test_track_publish_consume, test_raw_multi_frame_group → test_group_multi_frame
(or similar), and update all other test functions prefixed with test_raw_ to
test_track_ or test_group_ as appropriate so names match the current APIs (refer
to symbol names BroadcastProducer.publish, TrackProducer.append_group, and
group.consume in the file to locate related tests and ensure names reflect
"track" or "group" semantics).
In `@rs/moq-ffi/src/producer.rs`:
- Around line 82-93: The publish method currently hardcodes track priority to 0
(moq_lite::Track { name, priority: 0 }), which prevents callers from choosing
non-default prioritization; add an overload like publish_with_priority(name:
String, priority: u8) (or change publish to accept Option<u8>) so callers can
supply a priority, propagate that value into moq_lite::Track when creating the
track inside publish/publish_with_priority, and keep the existing publish(name)
as a convenience wrapper delegating to the new method to avoid breaking callers;
update construction locations where MoqTrackProducer is created so they call the
new API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcfdb2e5-01de-47d8-bf7f-23d6c42c636f
📒 Files selected for processing (6)
py/moq-lite/moq_lite/__init__.pypy/moq-lite/moq_lite/publish.pypy/moq-lite/moq_lite/subscribe.pypy/moq-lite/tests/test_local.pyrs/moq-ffi/src/consumer.rsrs/moq-ffi/src/producer.rs
✅ Files skipped from review due to trivial changes (1)
- py/moq-lite/moq_lite/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- py/moq-lite/moq_lite/subscribe.py
lullabee
changed the title
- moq-lite: TrackConsumer::read_frame() helper for one-frame-per-group tracks (moq-boy status/command pattern). Skips empty groups and returns None at end-of-track. - moq-ffi / py: MoqBroadcastProducer::publish -> publish_track, MoqBroadcastConsumer::subscribe -> subscribe_track, MoqTrackConsumer::next_group -> recv_group (arrival order). The name change tracks the moq-lite rename happening on a separate branch. - moq-ffi / py: MoqTrackConsumer::read_frame() exposed as a thin wrapper. - py: TrackConsumer.recv_group explicit method alongside async iteration; __anext__ now delegates to recv_group. - tests: 3 new cases for read_frame (one-per-group, multi-frame skip, end-of-track). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Exposes both arrival-order (recv_group) and sequence-order (next_group) variants on the FFI and Python track consumer APIs. next_group is wired to moq-lite's current next_group (arrival order) for now; will switch to the sequence-ordered method once it lands on moq-lite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- moq-ffi TrackInner::recv_group now calls moq_lite::TrackConsumer::recv_group (was calling the deprecated next_group). TrackInner::next_group now calls next_group_ordered, which is the sequence-ordered variant just landed on main. - moq-lite read_frame helper switched to recv_group; it only needs arrival order for its one-frame-per-group usage. - justfile: ci recipe now builds moq-ffi from source before pyright and passes --no-sync so uv doesn't replace it with the registry build. Same for pytest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…v#1326 Drops my loop-over-recv_group implementation in favor of the upstream scanner (poll_read_frame on State), which correctly skips past stalled earlier groups instead of blocking on them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kixelated
force-pushed
the
claire/pylib_rawdata
branch
from
ab9c96d to
b30b6c1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
py/moq-lite/tests/test_local.py (1)
282-305:⚠️ Potential issue | 🟡 MinorNarrow the
pytest.raisesexception types (Ruff B017, still unaddressed).Ruff still flags lines 282, 292, 301, and 304 as B017.
pytest.raises(Exception)will swallow unrelated failures (e.g.,AttributeErrorfrom a typo, import errors) and hide regressions in the new raw-track lifecycle code these tests are meant to guard. Tighten to the concrete FFI error class or at minimum addmatch=on the expected "closed/finished" message.🔧 Suggested tightening (adjust to the actual exception type)
- with pytest.raises(Exception): + with pytest.raises(Exception, match=r"(?i)finish|closed|done"): group.write_frame(b"too late")Apply the same pattern to lines 292, 301, and 304.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@py/moq-lite/tests/test_local.py` around lines 282 - 305, The tests use pytest.raises(Exception) which is too broad; update the four raises in test_raw_group_finish_twice_fails and test_raw_track_write_after_finish_fails to assert a specific error by replacing Exception with the concrete FFI error class used by your library (e.g., the FFI/interop error type) or, if uncertain, add a match="closed|finished" argument to pytest.raises to narrow the expectation. Locate the assertions around group.finish() in test_raw_group_finish_twice_fails and around track.write_frame(...) and track.append_group() in test_raw_track_write_after_finish_fails (objects created via BroadcastProducer.publish_track, track.append_group, group.finish, track.finish) and tighten each pytest.raises accordingly.
🧹 Nitpick comments (1)
rs/moq-ffi/src/consumer.rs (1)
156-182: UseMoqGroupConsumer::newto avoid duplicating the constructor logic.Both
recv_groupandnext_groupreproduce thesequence+Task::new(GroupInner { group })initialization inline, whileMoqGroupConsumer::new(defined at lines 213–220) does exactly that. Delegating keeps a single source of truth if the struct grows new fields.♻️ Proposed refactor
pub async fn recv_group(&self) -> Result<Option<Arc<MoqGroupConsumer>>, MoqError> { self.task .run(|mut state| async move { - Ok(state.recv_group().await?.map(|group| { - Arc::new(MoqGroupConsumer { - sequence: group.info.sequence, - task: Task::new(GroupInner { group }), - }) - })) + Ok(state.recv_group().await?.map(|group| Arc::new(MoqGroupConsumer::new(group)))) }) .await } pub async fn next_group(&self) -> Result<Option<Arc<MoqGroupConsumer>>, MoqError> { self.task .run(|mut state| async move { - Ok(state.next_group().await?.map(|group| { - Arc::new(MoqGroupConsumer { - sequence: group.info.sequence, - task: Task::new(GroupInner { group }), - }) - })) + Ok(state.next_group().await?.map(|group| Arc::new(MoqGroupConsumer::new(group)))) }) .await }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-ffi/src/consumer.rs` around lines 156 - 182, The two methods recv_group and next_group duplicate the MoqGroupConsumer construction; update both to call MoqGroupConsumer::new(group) inside the map instead of manually constructing with sequence and Task::new(GroupInner { group }), so the map becomes .map(|group| Arc::new(MoqGroupConsumer::new(group))) thereby centralizing initialization logic in MoqGroupConsumer::new.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@py/moq-lite/tests/test_local.py`:
- Around line 282-305: The tests use pytest.raises(Exception) which is too
broad; update the four raises in test_raw_group_finish_twice_fails and
test_raw_track_write_after_finish_fails to assert a specific error by replacing
Exception with the concrete FFI error class used by your library (e.g., the
FFI/interop error type) or, if uncertain, add a match="closed|finished" argument
to pytest.raises to narrow the expectation. Locate the assertions around
group.finish() in test_raw_group_finish_twice_fails and around
track.write_frame(...) and track.append_group() in
test_raw_track_write_after_finish_fails (objects created via
BroadcastProducer.publish_track, track.append_group, group.finish, track.finish)
and tighten each pytest.raises accordingly.
---
Nitpick comments:
In `@rs/moq-ffi/src/consumer.rs`:
- Around line 156-182: The two methods recv_group and next_group duplicate the
MoqGroupConsumer construction; update both to call MoqGroupConsumer::new(group)
inside the map instead of manually constructing with sequence and
Task::new(GroupInner { group }), so the map becomes .map(|group|
Arc::new(MoqGroupConsumer::new(group))) thereby centralizing initialization
logic in MoqGroupConsumer::new.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c15a63bf-ee0d-47a5-aa29-ad8d00d4131a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
justfilepy/moq-lite/moq_lite/__init__.pypy/moq-lite/moq_lite/publish.pypy/moq-lite/moq_lite/subscribe.pypy/moq-lite/pyproject.tomlpy/moq-lite/tests/test_local.pyrs/moq-ffi/src/consumer.rsrs/moq-ffi/src/origin.rsrs/moq-ffi/src/producer.rs
✅ Files skipped from review due to trivial changes (2)
- py/moq-lite/pyproject.toml
- py/moq-lite/moq_lite/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- rs/moq-ffi/src/origin.rs
- py/moq-lite/moq_lite/subscribe.py
The ci and test recipes ran `uv run maturin develop` (which only syncs the root workspace's dev group) and then `uv run --package moq-lite --no-sync`, so pytest (declared in moq-lite's dev group) was never installed. pyright failed with "Import \"pytest\" could not be resolved" in py/moq-lite/tests/test_local.py. Add an explicit `uv sync --package moq-lite` before the maturin step so pytest is present; the subsequent maturin develop overrides the registry moq-ffi with the source build, and --no-sync preserves it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
The change adds track- and group-level raw publishing and consumption across the Rust FFI and Python wrapper: new UniFFI types MoqTrackProducer, MoqGroupProducer, MoqTrackConsumer, MoqGroupConsumer and a MoqBroadcastConsumer::subscribe were introduced; MoqBroadcastProducer gained publish and a UniFFI consume. The Python package exposes GroupProducer, TrackProducer, GroupConsumer, TrackConsumer, extends BroadcastProducer/BroadcastConsumer with publish/subscribe and corresponding consume helpers, adds async iterators for groups/frames, includes new unit tests, and bumps version to 0.0.3.