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

Add option to update modules with linting #1588

Merged
merged 38 commits into from
Jun 2, 2022

Conversation

mirpedrol
Copy link
Member

Related to #1434

Add the --fix argument to linting of modules.
If --fix flag is provided, the module is updated to the latest version.

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

@mirpedrol mirpedrol added the WIP Work in progress label May 17, 2022
@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #1588 (358bb0a) into dev (a91cc78) will decrease coverage by 0.20%.
The diff coverage is 79.70%.

@@            Coverage Diff             @@
##              dev    #1588      +/-   ##
==========================================
- Coverage   64.65%   64.44%   -0.21%     
==========================================
  Files          54       54              
  Lines        6232     6288      +56     
==========================================
+ Hits         4029     4052      +23     
- Misses       2203     2236      +33     
Impacted Files Coverage Δ
nf_core/lint/actions_awsfulltest.py 88.88% <ø> (ø)
nf_core/lint/actions_awstest.py 83.33% <ø> (ø)
nf_core/lint/actions_ci.py 89.09% <ø> (ø)
nf_core/lint/modules_json.py 85.00% <ø> (ø)
nf_core/lint/multiqc_config.py 80.43% <ø> (ø)
nf_core/lint/schema_description.py 68.96% <ø> (ø)
nf_core/modules/lint/meta_yml.py 64.10% <ø> (ø)
nf_core/modules/lint/module_changes.py 76.00% <ø> (ø)
nf_core/modules/lint/module_todos.py 100.00% <ø> (ø)
nf_core/modules/lint/main_nf.py 65.65% <35.08%> (-10.46%) ⬇️
... and 35 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 a91cc78...358bb0a. Read the comment docs.

@mirpedrol mirpedrol removed the WIP Work in progress label May 19, 2022
@mirpedrol mirpedrol marked this pull request as ready for review May 19, 2022 12:29
@@ -594,7 +594,8 @@ def create_test_yml(ctx, tool, run_tests, output, force, no_prompts):
@click.option("-a", "--all", is_flag=True, help="Run on all modules")
@click.option("--local", is_flag=True, help="Run additional lint tests for local modules")
@click.option("--passed", is_flag=True, help="Show passed tests")
def lint(ctx, tool, dir, key, all, local, passed):
@click.option("--fix", is_flag=True, help="Fix the module version if a newer version is available")
Copy link
Member

Choose a reason for hiding this comment

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

Here I'm not sure if --fix alone is a good flag name or it would be better to call it --fix_version. When running the nf-core modules lint with a --fix option, I would expect that this fixes potentially all the linting errors and not only the tool version. What do you think?

Copy link
Member

@ggabernet ggabernet May 31, 2022

Choose a reason for hiding this comment

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

Or maybe in a similar way that nf-core lint --fix works, by providing which kind of linting failure should be fixed. E.g. nf-core modules lint --fix versions. Not sure if it is possible to implement like this in this case

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's true, it can also be misleading with the --fix option from the pipeline linting. I will do some refactoring to --fix_version

Copy link
Member Author

Choose a reason for hiding this comment

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

I've checked the functionality of the --fix flag in the pipeline linting and I think it's different from what we have here with the version, as it allows the different lint tests, but the version fix is included in the linting of main.nf.
I changed the name of the flag to --fix-version, and we can implement the option to run specific lint tests in a different PR, do you agree?

Copy link
Member

@ggabernet ggabernet left a comment

Choose a reason for hiding this comment

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

Apart from the comment I made, testing worked well for me:

nf-core modules lint --fix

                                          ,--./,-.
          ___     __   __   __   ___     /,-._.--~\
    |\ | |__  __ /  ` /  \ |__) |__         }  {
    | \| |       \__, \__/ |  \ |___     \`-._,-`-,
                                          `._,._,'

    nf-core/tools version 2.5.dev0 - https://nf-co.re


? Lint all modules or a single named module? Named module
? Tool name: kallisto/index
INFO     Linting modules repo: '.'                        __init__.py:170
INFO     Linting module: 'kallisto/index'                 __init__.py:174
INFO     Updating package 'bioconda::kallisto' 0.46.2 -> 0.48.0

╭───────────────────────╮
│ LINT RESULTS SUMMARY  │
├───────────────────────┤
│ [✔]  21 Tests Passed  │
│ [!]   0 Test Warnings │
│ [✗]   0 Tests Failed  │
╰───────────────────────╯

and I got these changes that I added to this PR, so all looks good to me!

@ggabernet ggabernet changed the base branch from dev to elixir June 2, 2022 12:45
@ggabernet ggabernet changed the base branch from elixir to dev June 2, 2022 12:45
Copy link
Member

@ggabernet ggabernet left a comment

Choose a reason for hiding this comment

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

I just have a minor thing for the changelog but looks good to me otherwise!

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Gisela Gabernet <gisela.gabernet@gmail.com>
CHANGELOG.md Outdated Show resolved Hide resolved
nf_core/modules/update.py Outdated Show resolved Hide resolved
@mirpedrol mirpedrol merged commit 69e8345 into nf-core:dev Jun 2, 2022
@mirpedrol mirpedrol deleted the update_modules branch June 2, 2022 13:31
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