Open mashehu opened 2 months ago
nf-core lint
overall result: Passed :white_check_mark: :warning:Posted for pipeline commit a09de95
+| ✅ 195 tests passed |+
#| ❔ 2 tests were ignored |#
!| ❗ 3 tests had warnings |!
Thank you @mashehu !! Thats really helpful 🙂
Thank you @mashehu ! Still very helpful!
Whoops I messed up and my mouse jumped 😆
Very nice code quality overall!
First of all thank you for reviewing! I will look into your comments and see how I can resolve them! 🙂
Some notes:
- For each module, you only have an
.nf
file. This is generally sufficient and fine for local modules, but you could better bundle the module resources by using a module directory with amain.nf
and adding the script as a template in the module directory, instead of having it inbin/
. This way you could get rid of all the argument parsing builerplate.
This is a good point and could maybe help with readability. As most of those scripts were developed as standalones, they were implemented as such. I will think about taking it into the refactoring for the metamap (see point 3), though maybe not for this release but a follow-up 1.1.!!
- When running the test configuration I saw that most modules don't provide a
tag
. Providing them would help users understanding what's going on.
A good point, probably very helpful, for users not so proficient, I will implement some tags!
- Related: You don't use a meta map at all. While this is generally acceptable as long as the modules are local, having them would make the pipeline more easily understandable for other nf-core developers. However this would require a lot of refactoring in the channel handling, so I think the current state is fine for now.
There was some development for the epitopeprediction modules within nf-core/epitopeprediction, and we wanted to align our prediction step, in this effort I would also implement the metamap, as well as refactor all module inputs, possibly as templates as mentioned above.
- I think your pipeline delivers quite some output that could be presented as a MultiQC report. Maybe you could implement this?
We decided actually against it, using multiqc as QC report and not output report. I will discuss again internally if we want to change that and the accordingly attach the figures (more downstream analysis, will probably come soon, so maybe together with the new plots?)
Did you want to add a comment on MultiQC? :D
@nictru We agreed to add a overview table to the multiqc: https://github.com/nf-core/metapep/pull/134. I will do the remaining comments next week and ping you again! Thank you again for reviewing!
Do not merge! This is a PR of dev compared to first release for whole-pipeline reviewing purposes. Changes should be made to
dev
and this PR should not be merged into first-commit-for-pseudo-pr!