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.

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

fix: linting with comments after the input directive #3458

Merged
merged 2 commits into from
Feb 20, 2025
Merged

fix: linting with comments after the input directive #3458

merged 2 commits into from
Feb 20, 2025
+2 −0

Conversation

nh13
Copy link
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 GitHub Actions
Copy link
Contributor

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 Codecov
Copy link

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
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
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 😉

nf_core/components/nfcore_component.py
@@ -206,6 +206,7 @@ def get_inputs_from_main_nf(self) -> None:
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
Contributor

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
Member Author

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
94 checks passed
@nh13 nh13 deleted the nh13-patch-1 branch February 20, 2025 08:04
@mashehu
Copy link
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
Member

@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