Closed ypriverol closed 1 year ago
master
branch :x:base
to dev
Hi @ypriverol,
It looks like this pull-request is has been made against the nf-core/quantms master
branch.
The master
branch on nf-core repositories should always contain code from the latest release.
Because of this, PRs to master
are only allowed if they come from the nf-core/quantms dev
branch.
You do not need to close this PR, you can change the target branch to dev
by clicking the "Edit" button at the top of this page.
Note that even after this, the test will continue to show as failing until you push a new commit.
Thanks again for your contribution!
nf-core lint
overall result: Passed :white_check_mark: :warning:Posted for pipeline commit d86ec32
+| ✅ 154 tests passed |+
#| ❔ 3 tests were ignored |#
!| ❗ 1 tests had warnings |!
@daichengxin @jpfeuffer can we duplicate the lfq test as test.config as suggested by @FriederikeHanssen .
Didn't we do that already?
I thought we duplicated it by registering it twice. We didn't duplicate the file. But I need to double check.
black
) is failingTo keep the code consistent with lots of contributors, we run automated code consistency checks. To fix this CI test, please run:
black
: pip install black
black .
Once you push these changes the test should pass, and you can hide this comment :+1:
We highly recommend setting up Black in your code editor so that this formatting is done automatically on save. Ask about it on Slack for help!
Thanks again for your contribution!
@ypriverol Can we check that the Changelog is uptodate? After that we will re-request reviews from nf-core people
@jpfeuffer everything is ready.
Hi @ewels @fabianegli @FriederikeHanssen @drpatelh Can you have a look to this PR.
Thanks!
I think this PR is from dev to master. The test only fails because the check was broken for a long time and the check on master is still from before when this was fixed in the template. Should not happen for the next release anymore.
Could you here, or in a separate issue or on Slack very quickly outline what is odd about the modules if you have time? Then we can handle that when addressing the modules. (Only if you have time and it can be digested into general comments).
We will fix the documentation.
Thanks!
I think this PR is from dev to master. The test only fails because the check was broken for a long time and the check on master is still from before when this was fixed in the template. Should not happen for the next release anymore.
Oh, my mistake. I saw the warning about the PR being against master and drew conclusions I didn't check up, sorry!
Could you here, or in a separate issue or on Slack very quickly outline what is odd about the modules if you have time? Then we can handle that when addressing the modules. (Only if you have time and it can be digested into general comments).
Actually, it's not so much. Besides the lack of conda, which was explained to me (maybe a comment in the code could be helpful for later reviewers, but I'm not sure what the guidelines for cases like this are) it's two things:
meta
object, is inconsistent and not the way most nf-core modules have them. As I say in one comment, one usually sees the meta in the first parameter like tuple val(meta), path(input)
. There also seems to be a preference for not having long tuples with all arguments, but instead separate (like you mostly have).Both of the above are certainly better described in the module guidelines.
We will fix the documentation.
Great. I'll approve this now.
My small comments have all been satisfactorily addressed.
Thanks a lot for your time. We are addressing most of the issues now.
nf-core lint
gives some warnings:
modules/local/preprocess_expdesign.nf │ Container versions do not match │
modules/local/preprocess_expdesign.nf │ Module does not emit software version │
modules/local/preprocess_expdesign.nf │ Process name not used for versions.yml │
modules/nf-core/custom/dumpsoftwareversions │ New version available │
modules/nf-core/multiqc │ New version available │
modules/local/preprocess_expdesign.nf │ Process label (process_very) is not among │
│ standard labels: │
│ process_single,process_low,process_medium,proc… │
modules/local/preprocess_expdesign.nf │ when: condition has been removed │
╵ │
@FriederikeHanssen Can you approve this PR. All comments have been addressed.
Can I merge @ewels @erikrikarddaniel @FriederikeHanssen ?
Can I merge @ewels @erikrikarddaniel @FriederikeHanssen ?
Fine for me. /D
PR checklist
nf-core lint
).nextflow run . -profile test,docker --outdir <OUTDIR>
).docs/usage.md
is updated.docs/output.md
is updated.CHANGELOG.md
is updated.README.md
is updated (including new tool citations and authors/contributors).