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

Check spaces in container links #2489

Merged
merged 6 commits into from
Oct 25, 2023
Merged

Conversation

fa2k
Copy link
Contributor

@fa2k fa2k commented Oct 23, 2023

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

This PR adds checks to see if there are spaces in the URLs used for container images - solving issue #2452 .
I'm adding tests for linting the container string, including the new check and the check for double quotes. (Similar to the tests for linting the process labels)

[Edited after merge - corrected issue number]

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.

It is looking good :) I added a couple of things that need fixing

CHANGELOG.md Outdated
@@ -50,6 +50,7 @@

- Add new command `nf-core subworkflows lint` ([#2379](https://github.com/nf-core/tools/pull/2379))
- Check for existence of test profile ([#2478](https://github.com/nf-core/tools/pull/2478))
- Check for spaces in container URLs ([#2452](https://github.com/nf-core/tools/issues/2452))
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
- Check for spaces in container URLs ([#2452](https://github.com/nf-core/tools/issues/2452))
- Check for spaces in modules container URLs ([#2452](https://github.com/nf-core/tools/issues/2452))

Could you move this point to the changelog for version 2.11dev above?

Comment on lines 268 to 269
1,
0,
Copy link
Member

Choose a reason for hiding this comment

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

This test should fail, not a warning

tests/modules/lint.py Show resolved Hide resolved
]


def test_modules_lint_check_process_labels(self):
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
def test_modules_lint_check_process_labels(self):
def test_modules_lint_check_url_spaces(self):

for test_case in CONTAINER_TEST_CASES:
process, passed, warned, failed = test_case
mocked_ModuleLint = MockModuleLint()
main_nf.check_process_section(
Copy link
Member

Choose a reason for hiding this comment

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

This function is checking the whole process section, including other checks such as labels, etc. In order to add a test for URL spaces only, you should write this check in a separate function, which is called inside check_process_section() and use this function here in the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! I will implement those changes. It's strange that my tests still passed, even though I had specified the test cases to be warnings instead of errors like they should be. I'll double check that the new tests were actually run - and make sure they fail when they require 1 warning and 0 errors.

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #2489 (3c6af2d) into dev (3fddb70) will increase coverage by 0.08%.
Report is 13 commits behind head on dev.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##              dev    #2489      +/-   ##
==========================================
+ Coverage   70.53%   70.62%   +0.08%     
==========================================
  Files          87       88       +1     
  Lines        9456     9493      +37     
==========================================
+ Hits         6670     6704      +34     
- Misses       2786     2789       +3     
Files Coverage Δ
nf_core/modules/lint/main_nf.py 69.75% <90.47%> (+1.36%) ⬆️

... and 9 files with indirect coverage changes

@fa2k fa2k force-pushed the check-spaces-in-container-links branch from 9e37803 to 3c6af2d Compare October 24, 2023 11:43
@fa2k
Copy link
Contributor Author

fa2k commented Oct 24, 2023

Those issues should now be fixed. (The subsequent force-push is a small correction to the test function name)

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 thanks!

@fa2k fa2k merged commit a2d69af into nf-core:dev Oct 25, 2023
18 of 19 checks passed
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.

2 participants