add strict mode feature, enable on test #99
Conversation
WalkthroughA new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
🧹 Nitpick comments (3)
README.md (1)
105-115: Doc nits: feature naming + env var semantics should match implementation.
- Line 105: “
bytesfeatures” → “bytesfeature”.- Lines 109-110: since code treats “env var is present” as strict, consider clarifying “set
MP4ATOM_STRICT(e.g. to1)” to avoid implying it parses truthy/falsey values.Proposed doc tweak
-There's also the `bytes` features which enables encoding for `Bytes` and `BytesMut` from the `bytes` crate, often used with tokio. +There's also the `bytes` feature which enables encoding for `Bytes` and `BytesMut` from the `bytes` crate, often used with tokio. Enable using the `strict` feature, or by setting the `MP4ATOM_STRICT` environment variable. +For example: `MP4ATOM_STRICT=1`.src/error.rs (1)
72-80: Optional: Consider usingvar_os()for cleaner env handling.The strict-mode logic is correct. If you add tests for
MP4ATOM_STRICTenvironment variable mutation, usevar_os("MP4ATOM_STRICT").is_some()instead ofvar(...).is_ok()to avoid UTF-8 validation and to more clearly express intent (presence enables strict, not a specific value). Extract this to a local variable to avoid repeated evaluation:pub(crate) fn decode_unknown(atom: &Any, parent: FourCC) -> Result<()> { - if std::env::var("MP4ATOM_STRICT").is_ok() || cfg!(feature = "strict") { + let strict = cfg!(feature = "strict") || std::env::var_os("MP4ATOM_STRICT").is_some(); + if strict { tracing::error!(kind = %atom.kind(), parent = %parent, "unexpected box"); return Err(Error::UnexpectedBox(atom.kind())); } else { tracing::warn!(kind = %atom.kind(), parent = %parent, "unexpected box"); } Ok(()) }justfile (1)
5-8: MP4ATOM_STRICT is a top-level export affecting all recipes, but only has runtime impact ontest. The environment variable controls strict parsing behavior indecode_unknown()(src/error.rs), turning warnings into hard errors. Sincecargo check,cargo clippy, andcargo fmtare static analysis tools that don't execute decode logic, the export doesn't functionally affect thecheckandfixrecipes. However, for clarity and to align with the documented use case ("useful... when testing mp4-atom itself"), consider scopingMP4ATOM_STRICTto only thetestrecipe:Scope MP4ATOM_STRICT to test recipe
export RUST_BACKTRACE := "1" export RUST_LOG := "info" -export MP4ATOM_STRICT := "1" # Run any CI tests test: + MP4ATOM_STRICT=1 cargo test --all-targets - cargo test --all-targets
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Cargo.tomlREADME.mdjustfilesrc/error.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Code must be clippy-clean (cargo clippy --all-targets --all-features -- -D warnings)
Code must be formatted with rustfmt (cargo fmt) and pass formatting checks (cargo fmt -- --check)
Write unit tests inline within modules using #[cfg(test)]
Files:
src/error.rs
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.rs: Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()
Use Header::read_from() to handle large atoms without loading entire contents into memory
All operations should return Result using custom error types defined in error.rs
Use the custom Buf/BufMut traits for contiguous byte slices instead of ad-hoc buffer handling
Files:
src/error.rs
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()
Applied to files:
justfileREADME.mdsrc/error.rs
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Use Header::read_from() to handle large atoms without loading entire contents into memory
Applied to files:
README.md
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/tokio/**/*.rs : For async IO (with the tokio feature), implement and use AsyncReadFrom and AsyncWriteTo traits
Applied to files:
README.md
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/any.rs : Register new atom types in the Any enum (src/any.rs) to enable flexible decoding
Applied to files:
src/error.rs
🔇 Additional comments (1)
Cargo.toml (1)
25-30: Feature flag addition looks correct.
strict = []is a clean opt-in switch forcfg!(feature = "strict").
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
I wouldn't do the env variable, but your call.
Actually I was a bit dubious about it, and only added it because of your comment "/// Either logs or returns an error depending on the environment/flag." Will chop that and just keep the feature flag. |
bradh
force-pushed
the
strict_mode_2026-01-10
branch
from
33704b4 to
88d5433
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
105-105: Minor grammar issue: subject-verb disagreement.Line 105 has "features" (plural) paired with "enables" (singular). Consider revising to either "feature which enables" or "features which enable" for grammatical consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Cargo.tomlREADME.mdjustfilesrc/error.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- justfile
- src/error.rs
- Cargo.toml
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()
Applied to files:
README.md
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/tokio/**/*.rs : For async IO (with the tokio feature), implement and use AsyncReadFrom and AsyncWriteTo traits
Applied to files:
README.md
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Use Header::read_from() to handle large atoms without loading entire contents into memory
Applied to files:
README.md
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/any.rs : Register new atom types in the Any enum (src/any.rs) to enable flexible decoding
Applied to files:
README.md
🔇 Additional comments (1)
README.md (1)
107-115: New Strict parsing section is clear and well-documented.The new "Strict parsing" section effectively explains the feature: what it does (treats unknown boxes as hard errors), why it's generally undesirable (ISO/IMBF tolerance for unknown boxes), and practical use cases (testing). The documentation follows the established pattern used for other features (e.g.,
tokio,bytes) and aligns with the implementation described in the PR.
Ah yeah, by environment I meant |
OK. I thought it might be worth running the tests in both modes (i.e. with and without the Would you prefer I did: (relative to the current PR configuration) |
Yeah, I had something like: pub(crate) fn decode_unknown(atom: &Any, parent: FourCC) -> Result<()> {
#[cfg(any(feature = "strict", test))]
return Err(Error::UnexpectedBox(atom.kind()));
#[cfg(not(any(feature = "strict", test)))]
{
tracing::warn!(kind = %atom.kind(), parent = %parent, "unexpected box");
return Ok(())
}
}@coderabbitai Can you show the correct syntax? It's probably wrong, just from memory. It's probably the same result as yours because the compiler will be smart enough inline the |
|
Tip For best results, initiate chat on the files or code changes. @kixelated, here's the corrected syntax using pub(crate) fn decode_unknown(atom: &Any, parent: FourCC) -> Result<()> {
#[cfg(any(feature = "strict", test))]
{
tracing::error!(kind = %atom.kind(), parent = %parent, "unexpected box");
return Err(Error::UnexpectedBox(atom.kind()));
}
#[cfg(not(any(feature = "strict", test)))]
{
tracing::warn!(kind = %atom.kind(), parent = %parent, "unexpected box");
Ok(())
}
}The key fix is wrapping the Alternative: bradh's approach using runtime checks is also valid and simpler: pub(crate) fn decode_unknown(atom: &Any, parent: FourCC) -> Result<()> {
if cfg!(feature = "strict") || cfg!(test) {
tracing::error!(kind = %atom.kind(), parent = %parent, "unexpected box");
Err(Error::UnexpectedBox(atom.kind()))
} else {
tracing::warn!(kind = %atom.kind(), parent = %parent, "unexpected box");
Ok(())
}
}You're correct that the compiler should optimize both to the same machine code since the conditions are known at compile time. The 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/error.rs (1)
72-81: Implementation looks correct; consider updating doc comment for completeness.The use of
cfg!(feature = "strict") || cfg!(test)correctly provides compile-time conditional behavior. The logic properly returns an error in strict/test mode while only warning in non-strict mode.Minor suggestion: The doc comment mentions only the "strict feature flag" but the function also enables strict behavior during tests via
cfg!(test). Consider updating for clarity:📝 Suggested doc comment update
-/// Either logs or returns an error depending on the strict feature flag. +/// Either logs or returns an error depending on the strict feature flag or test mode.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/error.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Code must be clippy-clean (cargo clippy --all-targets --all-features -- -D warnings)
Code must be formatted with rustfmt (cargo fmt) and pass formatting checks (cargo fmt -- --check)
Write unit tests inline within modules using #[cfg(test)]
Files:
src/error.rs
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.rs: Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()
Use Header::read_from() to handle large atoms without loading entire contents into memory
All operations should return Result using custom error types defined in error.rs
Use the custom Buf/BufMut traits for contiguous byte slices instead of ad-hoc buffer handling
Files:
src/error.rs
🧠 Learnings (3)
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/any.rs : Register new atom types in the Any enum (src/any.rs) to enable flexible decoding
Applied to files:
src/error.rs
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()
Applied to files:
src/error.rs
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : All operations should return Result<T> using custom error types defined in error.rs
Applied to files:
src/error.rs
🧬 Code graph analysis (1)
src/error.rs (1)
src/atom.rs (1)
decode_unknown(13-15)
2 participants
Replaces the remaining part of #92.