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

Refactor modules lint #1164

Merged
merged 13 commits into from
Jul 9, 2021
Merged

Conversation

ErikDanielsson
Copy link
Contributor

@ErikDanielsson ErikDanielsson commented Jul 6, 2021

Another try at refactoring nf-core modules lint and adding module tests to nf-core lint:

  • Moved code into methods and removed duplicated methods.
  • Added modules tests to nf-core lint and made the general lint table and the module lint table work together.
  • Created lint_utils.py for code used by both modules lint and lint commands.

On the way to resolving #1158. If these changes make sense, we should also update the markdown and json generation by nf-core lint to also include the modules tests.

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

@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #1164 (0060db2) into dev (004b2dd) will decrease coverage by 0.82%.
The diff coverage is 65.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1164      +/-   ##
==========================================
- Coverage   71.56%   70.74%   -0.83%     
==========================================
  Files          29       49      +20     
  Lines        3355     5233    +1878     
==========================================
+ Hits         2401     3702    +1301     
- Misses        954     1531     +577     
Impacted Files Coverage Δ
nf_core/licences.py 25.00% <0.00%> (-61.74%) ⬇️
nf_core/lint/actions_schema_validation.py 90.62% <ø> (ø)
nf_core/sync.py 51.79% <30.76%> (-0.70%) ⬇️
nf_core/modules/install.py 42.48% <42.48%> (ø)
nf_core/launch.py 64.93% <42.50%> (-2.10%) ⬇️
nf_core/__main__.py 56.29% <42.85%> (-7.89%) ⬇️
nf_core/modules/module_utils.py 45.18% <45.18%> (ø)
nf_core/modules/test_yml_builder.py 45.98% <45.98%> (ø)
nf_core/download.py 54.98% <46.47%> (-9.79%) ⬇️
nf_core/list.py 80.65% <54.16%> (-1.65%) ⬇️
... and 71 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42de39e...0060db2. Read the comment docs.

@KevinMenden KevinMenden mentioned this pull request Jul 8, 2021
4 tasks
self.lint_tests = [k for k in self.lint_tests if k in self.key]
# Filter the tests by the key if one is supplied
if key:
self.filter_tests_by_key()
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to supply the key to the filter function here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Went back and forth between having key as a argument and as an attribute of the class.

Comment on lines +191 to -214
def filter_tests_by_key(self, key):
"""Filters the tests by the supplied key"""
# Check that supplied test keys exist
bad_keys = [k for k in key if k not in self.lint_tests]
if len(bad_keys) > 0:
raise AssertionError(
"Test name{} not recognised: '{}'".format(
"s" if len(bad_keys) > 1 else "",
"', '".join(bad_keys),
)
mod_object.main_nf = mod
mod_object.module_name = os.path.basename(mod)
self.lint_module(mod_object, local=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only looking for bad keys right?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah nevermind, a bit downstream ...

@KevinMenden KevinMenden marked this pull request as ready for review July 9, 2021 09:16
Copy link
Contributor

@KevinMenden KevinMenden 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 to me and works locally when testing! 👍

Any further refactoring can still be handled after the v2.0 release then.

@ErikDanielsson ErikDanielsson merged commit 0478055 into nf-core:dev Jul 9, 2021
@ErikDanielsson ErikDanielsson mentioned this pull request Jul 14, 2021
4 tasks
@ErikDanielsson ErikDanielsson deleted the refactor-modules-lint branch July 26, 2022 06:05
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