-
Notifications
You must be signed in to change notification settings - Fork 143
Reduce the moq-lite API size #943
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis pull request refactors the moq crate ecosystem to depend on the external 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-native/src/server.rs (1)
288-298:⚠️ Potential issue | 🔴 CriticalBug:
close()will always panic due to unconditionalunreachable!().The
if letblocks on lines 290–296 do notreturnafter closing the backend, so execution always falls through tounreachable!()on line 297. Compare withtls_info()(line 162) andlocal_addr()(line 276) which correctly usereturninside theirif letblocks.🐛 Proposed fix
pub fn close(&mut self) { #[cfg(feature = "quinn")] if let Some(quinn) = self.quinn.as_mut() { quinn.close(); + return; } #[cfg(feature = "quiche")] if let Some(quiche) = self.quiche.as_mut() { quiche.close(); + return; } unreachable!("no QUIC backend compiled"); }
🤖 Fix all issues with AI agents
In `@rs/moq-native/src/lib.rs`:
- Around line 44-54: Add a compile-time check to prevent the QuicBackend enum
from being empty when neither QUIC feature is enabled: insert a cfg
compile_error guard before the QuicBackend definition (in
rs/moq-native/src/lib.rs) using #[cfg(not(any(feature = "quinn", feature =
"quiche")))] compile_error!(...) so that the crate fails to compile with a clear
message if neither feature is selected; this references the QuicBackend enum and
ensures at least one of the "quinn" or "quiche" features is enabled.
🧹 Nitpick comments (3)
rs/moq-lite/src/model/time.rs (1)
191-199: Consider documenting the relationship between these methods and the trait impls.The new
encode/decodemethods (lines 192–199) duplicate theEncode<V>/Decode<V>trait impls (lines 299–310) but with()hardcoded as the version andErrorinstead ofDecodeErroras the error type. This is a reasonable convenience API for callers who don't need versioned encoding, but the coexistence could be confusing to maintainers.A brief doc comment (e.g., "Version-less convenience wrapper; see also
Encode/Decodetrait impls") would help clarify intent.rs/moq-native/src/iroh.rs (1)
189-197: Consider adding a unit test forurl_set_scheme.This function uses string splitting to work around
Url::set_schemerestrictions. While the final.parse()validates the result, edge cases (e.g., URLs with userinfo containing:, opaque URLs) could produce surprising results. A few unit tests would provide confidence. As per coding guidelines, Rust tests should be integrated within source files.rs/moq-lite/src/ietf/parameters.rs (1)
250-298: Prefer removing dead code over commenting it out.Block comments (
/* ... */) around the unused constants and accessors are easy to miss and create visual noise. Consider deleting them outright — they remain recoverable from git history if ever needed again.
| #[derive(Clone, Debug, clap::ValueEnum, serde::Serialize, serde::Deserialize)] | ||
| #[serde(rename_all = "lowercase")] | ||
| pub enum QuicBackend { | ||
| /// [web-transport-quinn](https://crates.io/crates/web-transport-quinn) | ||
| #[cfg(feature = "quinn")] | ||
| Quinn, | ||
|
|
||
| /// [web-transport-quiche](https://crates.io/crates/web-transport-quiche) | ||
| #[cfg(feature = "quiche")] | ||
| Quiche, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
rg -n 'compile_error|cfg.*any.*quinn.*quiche' --type=rustRepository: moq-dev/moq
Length of output: 753
🏁 Script executed:
# Find and check the Cargo.toml for moq-native to see feature definitions
find . -name "Cargo.toml" -type f | xargs grep -l "moq-native" | head -5Repository: moq-dev/moq
Length of output: 161
🏁 Script executed:
# Check the moq-native Cargo.toml directly
cat rs/moq-native/Cargo.tomlRepository: moq-dev/moq
Length of output: 2380
🏁 Script executed:
# Search for QuicBackend usage in the codebase to understand if it's instantiated
rg -n "QuicBackend" --type=rust -A 2 -B 2Repository: moq-dev/moq
Length of output: 3897
Add compile-time check to ensure at least one QUIC backend is enabled.
The QuicBackend enum can become empty if neither quinn nor quiche features are enabled. Although downstream code using this enum is conditionally compiled to avoid instantiation in such cases, adding an explicit compile-time error in the library definition makes the contract clear and prevents confusing downstream errors. This is especially important for a library where users might construct unusual feature combinations.
Add the following check to rs/moq-native/src/lib.rs:
#[cfg(not(any(feature = "quinn", feature = "quiche")))]
compile_error!("At least one QUIC backend (quinn or quiche) must be enabled");🤖 Prompt for AI Agents
In `@rs/moq-native/src/lib.rs` around lines 44 - 54, Add a compile-time check to
prevent the QuicBackend enum from being empty when neither QUIC feature is
enabled: insert a cfg compile_error guard before the QuicBackend definition (in
rs/moq-native/src/lib.rs) using #[cfg(not(any(feature = "quinn", feature =
"quiche")))] compile_error!(...) so that the crate fails to compile with a clear
message if neither feature is selected; this references the QuicBackend enum and
ensures at least one of the "quinn" or "quiche" features is enabled.
No need to export ietf/lite and force a semver bump every single change.