Closed BrunoGrandePhD closed 2 years ago
โค๏ธ yes more than welcome addition. Thank you for contributing ๐ I agree, probably easiest to just prepend the process. I am trying to find time to convert this to DSL2, it might be easier to then use samtolls fastq
directly.
nf-core lint might be an issue tbh given the recent release for dsl2, this pipeline doesn't adhere to it anymore. So if linting fails because of DSL2 things, don't worry about it
Can you open the PR to dev
so we can follow the usual protocol? :)
I've updated the target branch to dev
and resolved the small merge conflict that arose from that.
I also installed nf-core tools v1.14 to lint using the pre-DSL2 version. I had to disable all checks related to the pipeline schema since it doesn't exist for bamtofastq
. You can compare the summaries from linting my branch (bgrande/cram-support
) with the results from linting the dev
branch. As you can see, my changes didn't introduce any additional warnings or failures. In fact, two more tests pass. ยฏ_(ใ)_/ยฏ
I've included the full linting report at the end of this comment. Let me know if there are any warnings/failures that I should address. In the meantime, I'll start documenting my changes as per the checklist.
โฏ nf-core --version
nf-core, version 1.14
โฏ cat .nf-core-lint.yml
schema_lint: False
schema_params: False
schema_description: False
# Run on bgrande/cram-support
โฏ nf-core lint .
[...]
โญโโโโโโโโโโโโโโโโโโโโโโโโฎ
โ LINT RESULTS SUMMARY โ
โโโโโโโโโโโโโโโโโโโโโโโโโค
โ [โ] 111 Tests Passed โ
โ [?] 14 Tests Ignored โ
โ [!] 12 Test Warnings โ
โ [โ] 36 Tests Failed โ
โฐโโโโโโโโโโโโโโโโโโโโโโโโฏ
# Run on upstream/dev
โฏ nf-core lint .
[...]
โญโโโโโโโโโโโโโโโโโโโโโโโโฎ
โ LINT RESULTS SUMMARY โ
โโโโโโโโโโโโโโโโโโโโโโโโโค
โ [โ] 109 Tests Passed โ
โ [?] 14 Tests Ignored โ
โ [!] 12 Test Warnings โ
โ [โ] 36 Tests Failed โ
โฐโโโโโโโโโโโโโโโโโโโโโโโโฏ
Looks great. I have triggered the tests now. Once they pass + some more documentation and I think we are good to go with merging
Okay, I went through the rest of the checklist and documented the new options for CRAM file support. Let me know if you have any suggested changes! I tentatively included this change as part of the 1.2.0
release. Feel free to change this as you see fit.
Thanks for sharing this workflow!
looks great. Like your selection of release name :D For the test file, I would propose we merge this PR now, and then update the path once its merged in an additional PR
I've opened two follow-up PRs to update the paths of the test CRAM files:
This aims to resolve #9. I opted for the simple approach of pre-pending the pipeline with a CRAM-to-BAM conversion, which has the advantage of leaving the rest of the pipeline intact. Some folks might want to be more efficient and convert directly to FASTQ, but I figured that it would be useful to have a less efficient implementation in the meantime.
I wanted to confirm whether this is a welcome change before going through the remaining tasks on the checklist. Thanks!
Edit: I forgot to mention that this is my first time writing Nextflow (other than trivial config changes), so I would gladly accept any feedback you might have!
PR checklist
testdata/
nextflow run . -profile test,docker
).nf-core lint .
).docs
is updatedCHANGELOG.md
is updatedREADME.md
is updatedLearn more about contributing: https://github.com/qbic-pipelines/bamtofastq/tree/master/.github/CONTRIBUTING.md