Skip to content

Fix strict syntax errors#2159

Merged
famosab merged 5 commits intodevfrom
fix-strict-syntax
Mar 31, 2026
Merged

Fix strict syntax errors#2159
famosab merged 5 commits intodevfrom
fix-strict-syntax

Conversation

@bentsherman
Copy link
Copy Markdown

@bentsherman bentsherman commented Mar 18, 2026

Fixing strict syntax errors so that I can test with Nextflow 26.04

The main change is removing if statements from config. You should be able to just remove them without generating tons of false warnings, since the strict syntax can validate process selectors against conditional processes as well.

I was able to run the test profile with these changes on 26.03.0-edge

@nf-core-bot
Copy link
Copy Markdown
Member

nf-core-bot commented Mar 18, 2026

Warning

Newer version of the nf-core template is available.

Your pipeline is using an old version of the nf-core template: 3.5.1.
Please update your pipeline to the latest version.

For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation.

bentsherman
bentsherman commented Mar 18, 2026
View reviewed changes
conf/modules/prepare_recalibration.config
if (params.use_gatk_spark) {
containerOptions = ''
}
containerOptions = params.use_gatk_spark ? '' : null
Copy link
Copy Markdown
Author

@bentsherman bentsherman Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like you are trying to clear out any previous container options when params.use_gatk_spark, in which case my change is probably wrong. So you might have to find a different way to make this work

bentsherman
bentsherman commented Mar 18, 2026
View reviewed changes
bentsherman
bentsherman commented Mar 18, 2026
View reviewed changes
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 18, 2026 •
edited
Loading

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 147b0ae

+| ✅ 224 tests passed       |+
#| ❔  13 tests were ignored |#
!| ❗   7 tests had warnings |!
Details

❗ Test warnings:

  • pipeline_todos - TODO string in base.config: Check the defaults for all processes
  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!
  • schema_lint - Input mimetype is missing or empty
  • schema_description - No description provided in schema for parameter: markduplicates_pixel_distance
  • schema_description - No description provided in schema for parameter: gatk_pcr_indel_model

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 3.5.1
  • Run at 2026-03-30 12:58:44

bentsherman
bentsherman commented Mar 18, 2026
View reviewed changes
FriederikeHanssen
FriederikeHanssen reviewed Mar 19, 2026
View reviewed changes
conf/modules/annotate.config

// SNPEFF
if (params.tools && (params.tools.split(',').contains('snpeff') || params.tools.split(',').contains('merge'))) {
withName: 'SNPEFF_SNPEFF' {
Copy link
Copy Markdown
Contributor

@FriederikeHanssen FriederikeHanssen Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is some sorcery around naming here in different modes, did you test that?

Copy link
Copy Markdown
Author

@bentsherman bentsherman Mar 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as far as I can tell, these if statements are just mirroring the pipeline logic for conditional processes, for example for this one:

sarek/subworkflows/local/vcf_annotate_all/main.nf

Lines 47 to 51 in 92b20b1

if (tools.split(',').contains('merge') || tools.split(',').contains('snpeff')) {
VCF_ANNOTATE_SNPEFF(vcf, snpeff_db, snpeff_cache)
vcf_ann = vcf_ann.mix(VCF_ANNOTATE_SNPEFF.out.vcf_tbi)
}

and I think the only purpose is to avoid false warnings about a process selector not matching any process

so, removing these if statements should not change any behavior:

  • if the process isn't run, the selector won't be used (and with strict syntax it won't report a warning either)
  • if the process is run, the selector will be applied to it same as before

in other pipelines I have seen examples of if-else, in which case you have to merge the two branches carefully, but I didn't see anything like that in sarek

Copy link
Copy Markdown
Author

@bentsherman bentsherman Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually there was an if-else in markduplicates.config, you can see how I merged the branches there. basically replace the if statement with a ternary for each config setting

ugly, but should become simpler in the future with things like workflow outputs

Copy link
Copy Markdown
Contributor

@FriederikeHanssen FriederikeHanssen Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I will review later today or tomorrow. The annotation subworkflow has a mmerge mode where snpeff and vep are both run sequentially, I'll double check the naming is fine there.

It looks like some of the tests are failing though.

Copy link
Copy Markdown
Author

@bentsherman bentsherman Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the failing tests are caused by some mutect2 outputs being published to the wrong location. Likely the ext / publishDir config was changed incorrectly

bentsherman
bentsherman commented Mar 19, 2026
View reviewed changes
bentsherman
bentsherman commented Mar 19, 2026
View reviewed changes
famosab
famosab reviewed Mar 27, 2026
View reviewed changes
famosab
famosab reviewed Mar 27, 2026
View reviewed changes
famosab
famosab reviewed Mar 27, 2026
View reviewed changes
famosab
famosab reviewed Mar 27, 2026
View reviewed changes
@famosab
Change mutect config to only have one CALCULATECONTAMINATION statement
286c917 
Co-authored-by: Famke Bäuerle <45968370+famosab@users.noreply.github.com>
famosab
famosab reviewed Mar 30, 2026
View reviewed changes
famosab
famosab reviewed Mar 30, 2026
View reviewed changes
famosab and others added 3 commits March 30, 2026 14:44
@famosab
Correct sentieon output naming
5edbbd7 
Co-authored-by: Famke Bäuerle <45968370+famosab@users.noreply.github.com>
@famosab
Remove obsolete joint_germline conf
1cd7eaf
@famosab
Update Changelog
147b0ae
famosab
famosab approved these changes Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@famosab famosab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @FriederikeHanssen @maxulysse maybe we can go ahead and merge this?

@famosab famosab merged commit b068220 into dev Mar 31, 2026
44 checks passed
@famosab famosab deleted the fix-strict-syntax branch March 31, 2026 08:37
@bentsherman
Copy link
Copy Markdown
Author

bentsherman commented Mar 31, 2026

Thanks @famosab for bringing this one over the finish line 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FriederikeHanssen FriederikeHanssen FriederikeHanssen left review comments

@famosab famosab famosab approved these changes

@maxulysse maxulysse Awaiting requested review from maxulysse maxulysse is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bentsherman @nf-core-bot @FriederikeHanssen @famosab