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

Removes tags.yml from module and subworkflow template #2995

Merged
merged 6 commits into from
Jun 21, 2024
Merged

Removes tags.yml from module and subworkflow template #2995

merged 6 commits into from
Jun 21, 2024

Conversation

adamrtalbot
Copy link
Contributor

@adamrtalbot adamrtalbot commented May 22, 2024 •
edited
Loading

tags.yml is no longer needed with newer testing workflows. This PR removes the file from the module and subworkflow templates and updates the linting tests to reflect this.

Note I've left tags in the templates because they're still useful.

Related to #2881

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

@adamrtalbot
Removes tags.yml from module and subworkflow template
33179ea 
tags.yml is no longer needed with newer testing workflows. This PR removes the file from the module and subworkflow templates and updates the linting tests to reflect this.

Related to nf-core#2881
@adamrtalbot
Copy link
Contributor Author

My ruff seems to lint differently to the default one, it's passing all pre-commits and linting but the code seems completely changed?

mirpedrol
mirpedrol reviewed May 22, 2024
View reviewed changes
nf_core/subworkflows/lint/subworkflow_tests.py Outdated
Comment on lines 224 to 233
if not is_pytest:
old_test_dir = Path(subworkflow.base_dir, "tests", "subworkflows", subworkflow.component_name)
if old_test_dir.is_dir():
subworkflow.failed.append(("test_old_test_dir", "old test directory exists", old_test_dir))
else:
subworkflow.passed.append(("test_old_test_dir", "old test directory does not exist", old_test_dir))
Copy link
Member

Choose a reason for hiding this comment

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

These lines should be kept, it's testing that a subworkflow with nf-test doesn't have a directory with pytests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

tests/modules/lint.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

The deleted tests should be removed from tests/test_modules.py, and the same from subworkflows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@mirpedrol
Copy link
Member

@nf-core-bot fix linting please!

mirpedrol
mirpedrol approved these changes May 22, 2024
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.

LGTM once tests pass :)
Are we planning to do a bulk PR to the models repo removing all tags.yml files? Otherwise we could keep a listing test throwing a warning, advising to remove this file.

@adamrtalbot
Copy link
Contributor Author

@nf-core-bot fix linting please!

@adamrtalbot
Copy link
Contributor Author

Are we planning to do a bulk PR to the models repo removing all tags.yml files? Otherwise we could keep a listing test throwing a warning, advising to remove this file.

I hadn't thought this far ahead, perhaps a warning would be better so we can do it piecemeal.

@adamrtalbot
Copy link
Contributor Author

@mirpedrol can you see what's wrong with the tests here? I'm getting a lot of import errors on my laptop which don't seem to match the errors on Github.

@mirpedrol
Copy link
Member

@adamrtalbot the tests which are failing now are also failing on the dev branch, I am working on finding what broke them, but it's not related to this PR

@codecov Codecov
Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 63.15789% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 76.03%. Comparing base (4ebbbb0) to head (03e78f9).

Files Patch % Lines
nf_core/modules/lint/module_tests.py 50.00% 4 Missing ⚠️
nf_core/subworkflows/lint/subworkflow_tests.py 62.50% 3 Missing ⚠️
Additional details and impacted files

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

@mirpedrol
Copy link
Member

@adamrtalbot all green now ✅

@adamrtalbot
Copy link
Contributor Author

@mirpedrol still unrelated failed tests. Any ideas?

@adamrtalbot adamrtalbot merged commit 200493c into nf-core:dev Jun 21, 2024
33 of 35 checks passed
@adamrtalbot adamrtalbot deleted the remove_tags.yml branch June 21, 2024 15:54
@GallVp GallVp mentioned this pull request Jul 16, 2024
Merged
17 tasks
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.
2 participants
@adamrtalbot @mirpedrol