-
Notifications
You must be signed in to change notification settings - Fork 751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clipkit - new module #6993
clipkit - new module #6993
Conversation
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.
Some comments :)
clipkit \\ | ||
$args \\ | ||
$aln | ||
|
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.
Maybe rename the clipkit file here to ${prefix}.clipkit
that way you are 100% sure it has the correct name
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 default functionality is for the tool to just add ".clipkit" to the input file name. We can either overwrite that within the script by passing the -o argument as suggested ( e.g. ${prefix}.clipkit ) or leave it to the user to do that from a nextflow.config through -args. I left it like this because I thought that just adding a suffix to the filename would be the most wanted functionality. Else to achieve this, the user would need to pass the filename as a meta.id, right?
Which is the preferred nf-core way to do this?
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.
I'd prefer the -o
option to stay consistent with all other modules. The input file name can sometimes be changed by another process in a way that the user doesn't want it. This way you can have full control over the file name in the nf-core best-practice way :)
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.
LGTM!
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