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

obtain module suggestions from installed modules #1624

Merged
merged 22 commits into from
Jul 5, 2022

Conversation

mirpedrol
Copy link
Member

@mirpedrol mirpedrol commented Jun 3, 2022

Close #1615

When nf-core modules test is run without specifying a module name, the suggestions from the prompt are obtained from the installed modules (instead of nf-core/modules repository).

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!

@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Merging #1624 (0b06097) into dev (ee54b9f) will increase coverage by 0.09%.
The diff coverage is 80.95%.

@@            Coverage Diff             @@
##              dev    #1624      +/-   ##
==========================================
+ Coverage   64.68%   64.78%   +0.09%     
==========================================
  Files          55       55              
  Lines        6338     6353      +15     
==========================================
+ Hits         4100     4116      +16     
+ Misses       2238     2237       -1     
Impacted Files Coverage Δ
nf_core/utils.py 80.48% <ø> (ø)
nf_core/modules/module_test.py 54.79% <80.95%> (+6.51%) ⬆️
nf_core/modules/modules_command.py 87.80% <0.00%> (+4.87%) ⬆️

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 ee54b9f...0b06097. Read the comment docs.

@mirpedrol mirpedrol marked this pull request as ready for review June 3, 2022 14:54
Copy link
Contributor

@fabianegli fabianegli left a comment

Choose a reason for hiding this comment

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

Hi @mirpedrol This already looks quite nice, but there are a few things that require some more attention.

Aside from the other comments, there should probably be a test to check if the local modules are collected. Such a test would have caught that all_local_modules was assigned, but not used.

nf_core/modules/module_test.py Outdated Show resolved Hide resolved
nf_core/modules/module_test.py Outdated Show resolved Hide resolved
nf_core/modules/module_test.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ErikDanielsson ErikDanielsson 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 in general, and I've made some comments on things that I think should change.

I don't think we should address this in this PR, but we need to think about if we want the --base-path flag to work when running modules commands on local custom modules repos. This would mean that one could specify in which directory the modules are located. We might be overloading the option a bit too much then, but since we are already doing that with the commands it might make sense.

nf_core/modules/module_test.py Show resolved Hide resolved
nf_core/modules/module_test.py Outdated Show resolved Hide resolved
nf_core/modules/module_test.py Show resolved Hide resolved
nf_core/modules/module_test.py Outdated Show resolved Hide resolved
nf_core/modules/module_test.py Outdated Show resolved Hide resolved
nf_core/modules/module_test.py Outdated Show resolved Hide resolved
nf_core/modules/modules_command.py Outdated Show resolved Hide resolved
Co-authored-by: Erik Danielsson <53212377+ErikDanielsson@users.noreply.github.com>
nf_core/modules/module_test.py Show resolved Hide resolved
nf_core/modules/module_test.py Show resolved Hide resolved
nf_core/modules/modules_command.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ErikDanielsson ErikDanielsson left a comment

Choose a reason for hiding this comment

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

LGTM!

@ewels ewels dismissed fabianegli’s stale review July 5, 2022 08:21

Comments now resolved

@ewels ewels merged commit 7eaeaf8 into nf-core:dev Jul 5, 2022
@mirpedrol mirpedrol deleted the module-selection branch July 5, 2022 08:33
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.

4 participants