fix: skip REF-only records #576
Conversation
WalkthroughThe changes in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
src/annotate/strucvars/mod.rs (1)
2962-2966: Adjust log level and remove unnecessary commaIf skipping REF-only records is expected behavior, consider changing the log level from
warningtoinfoto reduce log noise. Additionally, remove the unnecessary comma afterrecordin the logging statement.Apply this diff to implement the suggestions:
if record.alternate_bases().is_empty() { // REF-only, skip - tracing::warn!("skipping REF-only record {:?}", record,); + tracing::info!("skipping REF-only record {:?}", record); continue; }
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #576 +/- ##
==========================================
- Coverage 74.01% 74.01% -0.01%
==========================================
Files 26 26
Lines 9857 9863 +6
==========================================
+ Hits 7296 7300 +4
- Misses 2561 2563 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/annotate/strucvars/mod.rs (1)
2962-2968: LGTM with a minor optimization suggestionThe logic for skipping REF-only records is correct and the warning message is informative. However, there's a small optimization opportunity in the comparison.
Consider simplifying the comparison by using
==directly on the slice:- if record.alternate_bases().is_empty() - || record.alternate_bases().as_ref() == &["<*>".to_string()] + if record.alternate_bases().is_empty() + || record.alternate_bases().as_ref() == &[String::from("<*>")] { // REF-only, skip tracing::warn!("skipping REF-only record {:?}", record); continue; }This change avoids creating a new
Stringin the comparison, which is slightly more efficient.🧰 Tools
🪛 GitHub Check: clippy
[warning] 2963-2963: taken reference of right operand
warning: taken reference of right operand
--> src/annotate/strucvars/mod.rs:2963:16
|
2963 | || record.alternate_bases().as_ref() == &["<*>".to_string()]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--------------------
| |
| help: use the right value directly:["<*>".to_string()]
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
= note:#[warn(clippy::op_ref)]on by default
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/annotate/strucvars/mod.rs (2 hunks)
🧰 Additional context used
🪛 GitHub Check: clippy
src/annotate/strucvars/mod.rs
[warning] 2963-2963: taken reference of right operand
warning: taken reference of right operand
--> src/annotate/strucvars/mod.rs:2963:16
|
2963 | || record.alternate_bases().as_ref() == &["<*>".to_string()]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--------------------
| |
| help: use the right value directly:["<*>".to_string()]
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
= note:#[warn(clippy::op_ref)]on by default
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/annotate/strucvars/mod.rs (1)
2962-2968: LGTM with a minor optimization suggestionThe logic for skipping REF-only or empty ALT records is correct, and the warning log is appropriate. This change improves the robustness of the code by handling these edge cases.
However, there's a minor optimization that can be made in the comparison:
- if record.alternate_bases().is_empty() - || record.alternate_bases().as_ref() == &["<*>".to_string()] + if record.alternate_bases().is_empty() + || record.alternate_bases().as_ref() == ["<*>".to_string()]This change avoids taking a reference of the right operand, which is unnecessary in this case.
🧰 Tools
🪛 GitHub Check: clippy
[warning] 2963-2963: taken reference of right operand
warning: taken reference of right operand
--> src/annotate/strucvars/mod.rs:2963:16
|
2963 | || record.alternate_bases().as_ref() == &["<*>".to_string()]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--------------------
| |
| help: use the right value directly:["<*>".to_string()]
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
= note:#[warn(clippy::op_ref)]on by default
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/annotate/strucvars/mod.rs (2 hunks)
🧰 Additional context used
🪛 GitHub Check: clippy
src/annotate/strucvars/mod.rs
[warning] 2963-2963: taken reference of right operand
warning: taken reference of right operand
--> src/annotate/strucvars/mod.rs:2963:16
|
2963 | || record.alternate_bases().as_ref() == &["<*>".to_string()]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--------------------
| |
| help: use the right value directly:["<*>".to_string()]
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
= note:#[warn(clippy::op_ref)]on by default
🔇 Additional comments (1)
src/annotate/strucvars/mod.rs (1)
1892-1892: Consider usinganyhow::bail!instead ofpanic!As mentioned in a previous review, it's generally better to use
anyhow::bail!in library code instead ofpanic!. This allows callers to handle the error gracefully. Consider replacing thepanic!withanyhow::bail!like this:- panic!("Only one alternative allele is supported for SVs, got {} alternative alleles ({:?})", alleles.len(), alleles); + anyhow::bail!("Only one alternative allele is supported for SVs, got {} alternative alleles ({:?})", alleles.len(), alleles);This change will propagate the error up the call stack instead of terminating the program abruptly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/annotate/strucvars/mod.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/annotate/strucvars/mod.rs (1)
1892-1892: Previous comment still applies: Replacepanic!withanyhow::bail!The use of
panic!can cause the entire program to terminate unexpectedly. As mentioned in a previous review, consider usinganyhow::bail!to allow the error to be handled gracefully.
| || record.alternate_bases().as_ref() == ["<*>".to_string()] | ||
| { | ||
| // REF-only, skip | ||
| tracing::warn!("skipping REF-only / empty ALT record {:?}", record); |
There was a problem hiding this comment.
Avoid logging entire records to prevent potential sensitive data exposure
Logging the full record may inadvertently expose sensitive information. Consider logging only essential details, such as the record's position and reference sequence name, to avoid potential PII leakage.
Apply this diff to address the issue:
- tracing::warn!("skipping REF-only / empty ALT record {:?}", record);
+ tracing::warn!(
+ "skipping REF-only / empty ALT record at {}:{}",
+ record.reference_sequence_name(),
+ record.position()
+ );📝 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.
| tracing::warn!("skipping REF-only / empty ALT record {:?}", record); | |
| tracing::warn!( | |
| "skipping REF-only / empty ALT record at {}:{}", | |
| record.reference_sequence_name(), | |
| record.position() | |
| ); |
Skip REF-only (no alternative alleles) records
Summary by CodeRabbit
New Features
Bug Fixes
Tests