Skip to content
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.

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

Add linting for ifEmpty(null) #3411

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

lmReef
Copy link
Contributor

@lmReef lmReef commented Jan 21, 2025

Add pipeline and subworkflow linting for ifEmpty(null)

Resolves issue #2506

Description from original issue

There are two general cases for workflows to use the channel operator ifEmpty. The first is ifEmpty( [ ] ) to ensure a process executes, for example when an input file is optional (although this can be replaced by toList()). The second is when a channel should not be empty and throws an error ifEmpty { error ... }, e.g. reading from an empty samplesheet.

There are multiple examples of workflows that inject null objects into channels using ifEmpty (see https://github.com/search?q=org%3Anf-core+ifEmpty&type=code&p=2), which could cause unhandled null pointer exceptions as was the case in #1966.
The feature request is that linting throws up a warning when this operation is found.

Warning message example:
image

Implementation based on pipeline_todos as it's very similar (link to main file)

Uses regex pattern ifEmpty\(\s*null\s*\)

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

@lmReef lmReef self-assigned this Jan 21, 2025
@lmReef lmReef changed the title Add linting for if empty null dev Add linting for ifEmpty(null) Jan 21, 2025
@mashehu
Copy link
Contributor

mashehu commented Jan 21, 2025

@nf-core-bot fix linting

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.

Hi @lmReef, thanks for contributing!
It is looking good, but we should also add a test for this. You can find samples for other linting tests in tests/pipelines/lint, I think https://github.com/nf-core/tools/blob/main/tests/pipelines/lint/test_template_strings.py will be useful for this case

nf_core/pipelines/lint/pipeline_if_empty_null.py Outdated Show resolved Hide resolved
nf_core/pipelines/lint/pipeline_if_empty_null.py Outdated Show resolved Hide resolved
nf_core/pipelines/lint/pipeline_if_empty_null.py Outdated Show resolved Hide resolved
@lmReef
Copy link
Contributor Author

lmReef commented Jan 22, 2025

awesome thanks for the feedback, I'll make those changes

@lmReef
Copy link
Contributor Author

lmReef commented Jan 24, 2025

@nf-core-bot fix linting

Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 92.10526% with 3 lines in your changes missing coverage. Please review.

Project coverage is 76.27%. Comparing base (ed1667f) to head (aa679d5).
Report is 1 commits behind head on dev.

Files with missing lines Patch % Lines
nf_core/pipelines/lint/pipeline_if_empty_null.py 92.00% 2 Missing ⚠️
...ore/subworkflows/lint/subworkflow_if_empty_null.py 88.88% 1 Missing ⚠️
Additional details and impacted files

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

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.

LGTM! 🙂

file_paths = []
pattern = re.compile(r"ifEmpty\s*\(\s*null\s*\)")

# Pipelines don"t provide a path, so use the workflow path.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Pipelines don"t provide a path, so use the workflow path.
# Pipelines don't provide a path, so use the workflow path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants