Updated nf-core modules remove to remove entry in modules.json
#1132
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1132 +/- ##
==========================================
- Coverage 70.58% 70.57% -0.01%
==========================================
Files 48 48
Lines 4827 4843 +16
==========================================
+ Hits 3407 3418 +11
- Misses 1420 1425 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks good @ErikDanielsson ! Just a few tiny suggestions otherwise good to go, thanks! 👍
nf_core/modules/remove.py
Outdated
| return questionary.confirm( | ||
| f"Do you wish to continue removing module '{module}'?", default=False | ||
| ).unsafe_ask() |
There was a problem hiding this comment.
I would say we don't need a prompt here, maybe just say that a modules.json is missing.
There was a problem hiding this comment.
Yeah, I guess we can rely on the user knowing rm -rf atleast.
nf_core/modules/remove.py
Outdated
| if not self.remove_modules_json_entry(module): | ||
| return False |
There was a problem hiding this comment.
Similar to the other command, I would rather print that the module entry couldn't be found in the modules.json and not prompt for it.
nf_core/modules/remove.py
Outdated
| modules_json_path = os.path.join(self.dir, "modules.json") | ||
| with open(modules_json_path, "r") as fh: | ||
| modules_json = json.load(fh) |
There was a problem hiding this comment.
We should probably verify that the modules.json and catch the exception if it doesn't to still remove the module. Just to be sure, for the initial phase ... maybe some people won't have the file. Although I think that the transition will be really fast.
There was a problem hiding this comment.
I thought about it, but didn't add it in the end since we check for the file in has_valid_pipeline. But I guess it doesn't do any harm checking for it, and if the code is changed at some point it might cause errors.
Updated
nf-core modules removeto remove entry inmodules.jsonas suggested in #1115. If entry is missing frommodules.jsonthe user is asked if they want to continue removing the module directory.PR checklist
CHANGELOG.mdis updateddocsis updated