-
Notifications
You must be signed in to change notification settings - Fork 192
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 nf-core modules
commands
#1124
Refactor nf-core modules
commands
#1124
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## dev #1124 +/- ##
==========================================
+ Coverage 70.41% 70.58% +0.16%
==========================================
Files 45 48 +3
Lines 4797 4827 +30
==========================================
+ Hits 3378 3407 +29
- Misses 1419 1420 +1
Continue to review full report at Codecov.
|
Hi @ErikDanielsson , overall I think this is a nice idea! One thing that is a bit tricky with the modules commands though is that some of them (mostly But other than that I think this is nice! But would be nice to hear what @ewels thinks about this :) |
Thanks, I'll take a look at the lint code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my phone, didn't get very far with the review sorry. But concept sounds great! I'm happy to merge if it LGTM you guys. Especially as high risk for merge conflicts.
nf_core/modules/install.py
Outdated
def install(self, module): | ||
if self.repo_type == "modules": | ||
log.error("You cannot install a module in a clone of nf-core/modules") | ||
sys.exit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we raise an exception instead and catch it in the cli code? I prefer not to have exits anywhere but there if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other checks just return false. So that would also be fine.
Refactored
pipeline_modules.py
into one file file pernf-core modules
command.pipeline_modules.py
ModuleCommand
formodules
commands to inherit from.list
,install
andremove
commands.The
modules
commands are all now found in<command>.py
.PR checklist
CHANGELOG.md
is updateddocs
is updated