Rename catalog task APIs to subscribe/unsubscribe#1140
Rename catalog task APIs to subscribe/unsubscribe#1140
Conversation
Disambiguate the two kinds of catalog handles (task vs snapshot) by renaming the task-side functions to use subscribe/unsubscribe terminology: - moq_consume_catalog() → moq_consume_catalog_subscribe() - moq_consume_catalog_close() → moq_consume_catalog_unsubscribe() - moq_consume_catalog_free() → moq_consume_catalog_close() Closes #1064 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe catalog consumption C API in libmoq was reorganized: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (Beta)
✨ 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.
🧹 Nitpick comments (1)
rs/libmoq/src/api.rs (1)
392-396: Rename unsubscribe handle variable totask(orcatalog_task) to complete disambiguation.
moq_consume_catalog_unsubscribecurrently names its handlecatalog, which can still mislead callers into passing snapshot IDs.♻️ Proposed fix
-pub extern "C" fn moq_consume_catalog_unsubscribe(catalog: u32) -> i32 { +pub extern "C" fn moq_consume_catalog_unsubscribe(task: u32) -> i32 { ffi::enter(move || { - let catalog = ffi::parse_id(catalog)?; - State::lock().consume.catalog_unsubscribe(catalog) + let task = ffi::parse_id(task)?; + State::lock().consume.catalog_unsubscribe(task) }) }Please mirror this naming in
rs/libmoq/src/consume.rsandrs/libmoq/README.mdfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/libmoq/src/api.rs` around lines 392 - 396, Rename the ambiguous "catalog" handle in moq_consume_catalog_unsubscribe to "task" (or "catalog_task"): change the function parameter name from catalog: u32 to task: u32, update the local let catalog = ffi::parse_id(catalog)? to let task = ffi::parse_id(task)? (or let catalog_task = ...), and use that new variable when calling State::lock().consume.catalog_unsubscribe(...). Apply the same renaming for the corresponding handle variable in the consume module (functions in consume.rs that accept or document this handle) and update the handle name in rs/libmoq/README.md so examples and docs use "task" or "catalog_task" consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rs/libmoq/src/api.rs`:
- Around line 392-396: Rename the ambiguous "catalog" handle in
moq_consume_catalog_unsubscribe to "task" (or "catalog_task"): change the
function parameter name from catalog: u32 to task: u32, update the local let
catalog = ffi::parse_id(catalog)? to let task = ffi::parse_id(task)? (or let
catalog_task = ...), and use that new variable when calling
State::lock().consume.catalog_unsubscribe(...). Apply the same renaming for the
corresponding handle variable in the consume module (functions in consume.rs
that accept or document this handle) and update the handle name in
rs/libmoq/README.md so examples and docs use "task" or "catalog_task"
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58546526-3a41-48b2-acf8-1a32e7201400
📒 Files selected for processing (4)
rs/libmoq/README.mdrs/libmoq/src/api.rsrs/libmoq/src/consume.rsrs/libmoq/src/test.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/libmoq/src/api.rs (1)
393-397: Consider renaming the parameter for clarity.Since the PR's goal is to disambiguate task handles from snapshot handles, the parameter
catalogin this function could be renamed tosubscriptionortaskto make it clearer that this takes the handle returned bymoq_consume_catalog_subscribe, not a snapshot handle from the callback.💡 Optional: rename parameter
-pub extern "C" fn moq_consume_catalog_unsubscribe(catalog: u32) -> i32 { +pub extern "C" fn moq_consume_catalog_unsubscribe(subscription: u32) -> i32 { ffi::enter(move || { - let catalog = ffi::parse_id(catalog)?; - State::lock().consume.catalog_unsubscribe(catalog) + let subscription = ffi::parse_id(subscription)?; + State::lock().consume.catalog_unsubscribe(subscription) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/libmoq/src/api.rs` around lines 393 - 397, Rename the parameter `catalog` in the function `moq_consume_catalog_unsubscribe` to a clearer name like `subscription` (or `task`) to reflect that it accepts the handle returned by `moq_consume_catalog_subscribe` rather than a snapshot handle; update the local binding passed to `ffi::parse_id` (e.g., `let subscription = ffi::parse_id(subscription)?;`) and update the call into `State::lock().consume.catalog_unsubscribe(subscription)` so the variable name matches and intent is unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rs/libmoq/src/api.rs`:
- Around line 393-397: Rename the parameter `catalog` in the function
`moq_consume_catalog_unsubscribe` to a clearer name like `subscription` (or
`task`) to reflect that it accepts the handle returned by
`moq_consume_catalog_subscribe` rather than a snapshot handle; update the local
binding passed to `ffi::parse_id` (e.g., `let subscription =
ffi::parse_id(subscription)?;`) and update the call into
`State::lock().consume.catalog_unsubscribe(subscription)` so the variable name
matches and intent is unambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dccfbc54-7bf5-47da-8030-7fc05b033dab
📒 Files selected for processing (2)
rs/libmoq/src/api.rsrs/libmoq/src/consume.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/libmoq/src/consume.rs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
moq_consume_catalog()→moq_consume_catalog_subscribe()moq_consume_catalog_close()→moq_consume_catalog_unsubscribe()moq_consume_catalog_free()→moq_consume_catalog_close()This disambiguates the two kinds of catalog
u32handles (task vs snapshot) which previously both had "catalog" in the name with unclear close/free semantics.Closes #1064
Test plan
cargo check -p libmoqpassescargo test -p libmoq— all 14 tests passjust checkpasses🤖 Generated with Claude Code