+
Skip to content

Support modules with exec: blocks #3633

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Aug 1, 2025
Merged

Support modules with exec: blocks #3633

merged 4 commits into from
Aug 1, 2025

Conversation

muffato
Copy link
Member

@muffato muffato commented Jun 20, 2025

With the current version, linting a module that has an exec: block yields the following error

╭─ [✗] 1 Module Test Failed ───────────────────────────────────────────────────╮
│                      ╷                          ╷                            │
│ Module name          │ File path                │ Test message               │
│╶─────────────────────┼──────────────────────────┼───────────────────────────╴│
│ hiccramalign/chunks  │ modules/sanger-tol/hicc… │ when: condition has too    │
│                      │                          │ many lines                 │
│                      ╵                          ╵                            │
╰──────────────────────────────────────────────────────────────────────────────╯

This is because the parser is not aware of exec: blocks.

In this PR I simply add code to recognise exec: blocks and check that they don't exist together with script: or shell:

Best,
Matthieu

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@codecov Codecov
Copy link

codecov bot commented Jun 20, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.29%. Comparing base (9bff2b9) to head (2872af9).
⚠️ Report is 5 commits behind head on dev.

Files with missing lines Patch % Lines
nf_core/modules/lint/main_nf.py 50.00% 4 Missing ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@muffato muffato mentioned this pull request Jul 15, 2025
Merged
mirpedrol
mirpedrol approved these changes Jul 30, 2025
View reviewed changes
Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

Code changes look good to me, thanks!
But I would like to have a final approve from someone from @nf-core/maintainers to make sure that I am not missing something, and we don't want to allow an exec block in nf-core modules.

@jfy133
Copy link
Member

jfy133 commented Jul 30, 2025

I think there is no explicit reason not to support it but :

  1. make sure that it is only recommend to use in certain contexts (e.g. not heavy jobs, as it runs on the head node)
  2. we need to ensure that the structure of the block itself is consistent... (harshil align etc)

For now I think it's fine to merge this (exec: is valid Nextflow), but we should have the maintainers team develop a couple extra points for the specs page :)

@jfy133
Copy link
Member

jfy133 commented Jul 31, 2025

So far it seems people are half neutral half against supporting within nf-core/modules officially, but I dont think linting should complain about a valid nextflow syntax so again I would merge this now so people can happily have local modules

@SPPearce
Copy link
Contributor

I agree with James's summary, we should support this in general but an nf-core module would need a convincing argument as to why we need exec there.

@FriederikeHanssen
Copy link
Contributor

FriederikeHanssen commented Jul 31, 2025 •
edited by jfy133
Loading

complain about a valid nextflow syntax

agreed, maybe as an improved later on, we can have stricter linting on the nf-core side to at least warn for execs (to Simon's point), but allow disableing this linting in a config file

@SPPearce
Copy link
Contributor

complain about a valid nextflow syntax

agreed, maybe as an improved later on, we can have stricter linting on the nf-core side to at least warn for execs (to Simon's point), but allow disableing this linting in a config file

I think it is fine in nf-core pipelines, just not in the modules repo personally.

@FriederikeHanssen
Copy link
Contributor

ah correct, I meant modules linting only

@mirpedrol
Copy link
Member

thanks for all the feedback! I am going to merge this PR then :)

@mirpedrol mirpedrol merged commit 1e3a7da into dev Aug 1, 2025
102 checks passed
@mirpedrol mirpedrol deleted the module_exec branch August 1, 2025 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mirpedrol mirpedrol mirpedrol approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@muffato @jfy133 @SPPearce @FriederikeHanssen @mirpedrol @nf-core-bot
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载