Rename catalog task APIs to subscribe/unsubscribe#1140
Open
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>
Contributor
WalkthroughThe C API for catalog consumption in libmoq was changed: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
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}
Contributor
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
Contributor
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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