-
Notifications
You must be signed in to change notification settings - Fork 752
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Added parallel version of ltrharvest #4968
Conversation
modules/nf-core/ltrharvest/meta.yml
Outdated
tools: | ||
- "edta": | ||
description: Extensive de-novo TE Annotator (EDTA) | ||
homepage: "https://github.com/oushujun/EDTA" | ||
documentation: "https://github.com/oushujun/EDTA" | ||
tool_dev_url: "https://github.com/oushujun/EDTA" | ||
doi: "10.1186/s13059-019-1905-y" | ||
licence: ["GPL v3"] | ||
- "gt": | ||
description: "The GenomeTools genome analysis system" | ||
homepage: "https://genometools.org/index.html" | ||
documentation: "https://genometools.org/documentation.html" | ||
tool_dev_url: "https://github.com/genometools/genometools" | ||
doi: "10.1109/TCBB.2013.68" | ||
licence: ["ISC"] |
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.
This is unusual, but I guess it makes sense.
Include an entry specifically for 'ltrharvest' so it's origin is clear
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.
Thank you @mahesh-panchal
I have added an entry for LTR_harvest_parallel.
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.
minor nitpick about simplifying test snapshot, otherwise looks good
assertAll( | ||
{ assert process.success }, | ||
{ assert snapshot(process.out).match() }, | ||
{ assert snapshot(path(process.out.versions[0]).text).match("stub_versions") } |
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.
the stub_versions
and script_versions
snapshots look the same. I think you can share those snapshots between test cases (.match("versions")
) since the script and stub blocks use the exact same logic
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.
it would also be ok to remove this check entirely or loosen it a bit to just check for existence, so you don't need to update the snapshots for minor version bumps... but its fine if you want to be conservative here
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.
Hi @ahvigil
Thank you for the feedback. It is not possible anymore to use the same snapshot name. nf-test doesn't allow that anymore.
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.
ugh thats too bad but it seems you're right: askimed/nf-test#188
however, there is an open issue to provide an opt in for cases like this where it might be useful to remove repetition: askimed/nf-test#197. Hope they can implement something like that!
* Added parallel version of ltrharvest * Removed scn from snapshot * Added entry for LTR_HARVEST_parallel in meta.yml
* Added parallel version of ltrharvest * Removed scn from snapshot * Added entry for LTR_HARVEST_parallel in meta.yml
PR checklist
Closes #XXX
versions.yml
file.label
nf-core modules test <MODULE> --profile docker
nf-core modules test <MODULE> --profile singularity
nf-core modules test <MODULE> --profile conda
nf-core subworkflows test <SUBWORKFLOW> --profile docker
nf-core subworkflows test <SUBWORKFLOW> --profile singularity
nf-core subworkflows test <SUBWORKFLOW> --profile conda