Skip to content

fix: linting with comments after the input directive#3458

Merged
nh13 merged 2 commits intodevfrom
nh13-patch-1
Feb 20, 2025
Merged

fix: linting with comments after the input directive#3458
nh13 merged 2 commits intodevfrom
nh13-patch-1

Conversation

@nh13
Copy link
Copy Markdown
Member

@nh13 nh13 commented Feb 20, 2025 •
edited
Loading

Fixes: #3457

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 20, 2025

This PR is against the main branch ❌

  • Do not close this PR
  • Click Edit and change the base to dev
  • This CI test will remain failed until you push a new commit

Hi @nh13,

It looks like this pull-request is has been made against the nf-core/tools main branch.
The main branch on nf-core repositories should always contain code from the latest release.
Because of this, PRs to main are only allowed if they come from the nf-core/tools dev branch.

You do not need to close this PR, you can change the target branch to dev by clicking the "Edit" button at the top of this page.
Note that even after this, the test will continue to show as failing until you push a new commit.

Thanks again for your contribution!

@nh13 nh13 changed the base branch from main to dev February 20, 2025 02:08
@nh13 nh13 closed this Feb 20, 2025
@nh13 nh13 reopened this Feb 20, 2025
@nh13 nh13 mentioned this pull request Feb 20, 2025
Merged
17 tasks
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 20, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.53%. Comparing base (22eb20c) to head (b253681).
Report is 3 commits behind head on dev.

Additional details and impacted files

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

@nh13 nh13 enabled auto-merge February 20, 2025 02:34
@nh13
Copy link
Copy Markdown
Member Author

nh13 commented Feb 20, 2025

@nf-core-bot changelog

@nh13 nh13 added the linting label Feb 20, 2025
mashehu
mashehu approved these changes Feb 20, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@mashehu mashehu left a comment

Choose a reason for hiding this comment

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

Looks good. @mirpedrol do you think this could result in some false positives?

Bonus points if you could add a test for this, @nh13 😉

Comment thread nf_core/components/nfcore_component.py
input_data = data.split("input:")[1].split("output:")[0]
for line in input_data.split("\n"):
channel_elements: Any = []
line = line.split("//")[0] # remove any trailing comments
Copy link
Copy Markdown
Contributor

@mashehu mashehu Feb 20, 2025

Choose a reason for hiding this comment

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

Suggested change
line = line.split("//")[0] # remove any trailing comments
line = line.split(" //")[0] # remove any trailing comments

make it a bit more error proof

Copy link
Copy Markdown
Member Author

@nh13 nh13 Feb 20, 2025

Choose a reason for hiding this comment

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

Why do we care about leading spaces? See:

tools/nf_core/modules/lint/main_nf.py

Line 577 in 52e8109

line = line_raw.split("//")[0]

@nh13 nh13 merged commit 1e63caf into dev Feb 20, 2025
@nh13 nh13 deleted the nh13-patch-1 branch February 20, 2025 08:04
@mashehu
Copy link
Copy Markdown
Contributor

mashehu commented Feb 20, 2025

woops! Didn't see the auto-merge... @nh13 any chance you could open one more PR with the suggested changes?

@mirpedrol
Copy link
Copy Markdown
Member

mirpedrol commented Feb 20, 2025

@mirpedrol do you think this could result in some false positives?

Looks good, I think it's unlikely to find // outside of a comment in this context.

@nh13 nh13 mentioned this pull request Feb 20, 2025
Closed
@mirpedrol mirpedrol mentioned this pull request Feb 21, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mashehu mashehu mashehu approved these changes

Assignees

No one assigned

Labels

linting

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Comments after the input directive in the main.nf yields a false positive in the linter

4 participants

@nh13 @mashehu @mirpedrol @nf-core-bot