refactor: centralize blocked flags + fix missing guards from PR #36 review#48
Merged
refactor: centralize blocked flags + fix missing guards from PR #36 review#48
Conversation
Replace duplicated blocked-flags logic (10+ lines x 5 modules) with
a single centralized registry in lib/BlockedFlags.groovy. Each module
now uses one line: `args = BlockedFlags.strip('MODULE_NAME', args, log)`
Fixes from PR #36 review:
- Add --no-prot-inf to ASSEMBLE, INDIVIDUAL, FINAL blocked lists
- Add --channel-run-norm, --channel-spec-norm to FINAL blocked list
- Add --var-mod, --fixed-mod, --channels to INSILICO (via COMMON)
- Add --relaxed-prot-inf, --pg-level to ASSEMBLE blocked list
- Add version guard for channel normalization flags (require >= 2.0)
- Add warning when --normalize false conflicts with channel norm flags
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
Add comments explaining: - Why blocked flags exist (prevent silent DIA-NN flag conflicts) - Why they live in a Groovy class, not config files (safety — can't be overridden by user configs) - How to add new blocked flags (edit one file, no module changes) - In each module: point developers to lib/BlockedFlags.groovy Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors DIA-NN --extra_args sanitization by centralizing all pipeline-managed/blocked DIA-NN flags into a shared lib/BlockedFlags.groovy helper, and incorporates additional DIA-NN v2.0+ guarding plus a warning for potentially contradictory normalization settings in the DIA workflow.
Changes:
- Introduces
lib/BlockedFlags.groovyas a single source of truth for blocked DIA-NN flags and shared stripping logic. - Updates DIA-NN local modules to call
BlockedFlags.strip(<MODULE>, args, log)instead of duplicating per-module regex stripping logic. - Extends DIA workflow version guarding to include channel normalization flags and adds a warning for
--no-normvs channel normalization contradiction.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| workflows/dia.nf | Adds DIA-NN v2.0+ guard for channel norm flags and warns on contradictory normalization settings. |
| modules/local/diann/preliminary_analysis/main.nf | Replaces in-module blocked-flag stripping with centralized BlockedFlags.strip. |
| modules/local/diann/insilico_library_generation/main.nf | Replaces in-module blocked-flag stripping with centralized BlockedFlags.strip. |
| modules/local/diann/individual_analysis/main.nf | Replaces in-module blocked-flag stripping with centralized BlockedFlags.strip. |
| modules/local/diann/final_quantification/main.nf | Replaces in-module blocked-flag stripping with centralized BlockedFlags.strip. |
| modules/local/diann/assemble_empirical_library/main.nf | Replaces in-module blocked-flag stripping with centralized BlockedFlags.strip. |
| lib/BlockedFlags.groovy | New centralized registry and implementation for blocked DIA-NN flags stripping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ASSEMBLE_EMPIRICAL_LIBRARY doesn't set --no-prot-inf, --relaxed-prot-inf, or --pg-level in its command, so blocking them would strip user values without providing a replacement. Users should be able to pass these to the assembly step if needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Each blocked flag now has a comment explaining WHY it's blocked: - "Pipeline-managed": the pipeline sets it from params/SDRF/metadata - "No-effect guard": the flag has no effect in this step but is blocked to prevent users from wrongly believing it does This prevents future contributors (human or AI) from removing flags without understanding the intent behind the block. Reverts the accidental removal of protein inference flags from ASSEMBLE_EMPIRICAL_LIBRARY — they are intentionally blocked as a no-effect guard since --gen-spec-lib produces a spectral library, not a quantified report. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
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
lib/BlockedFlags.groovy— single source of truthReview fixes included
--no-prot-infmissing from ASSEMBLE, INDIVIDUAL, FINAL--channel-run-norm,--channel-spec-normmissing from FINAL--no-norm+channel_run_normcontradiction--var-mod,--fixed-mod,--channelsArchitecture
BlockedFlags.groovydefines:COMMONlist: flags shared by all DIA-NN stepsMODULE_FLAGSmap: per-module additionsstrip(): the shared stripping logic with warningsTest plan
test_dia— verify args stripping still works--extra_args "--no-prot-inf"— verify warning appears for all modules--channel_run_norm truewith DIA-NN 1.8.1 — verify version guard error🤖 Generated with Claude Code