[WIP] --tools is now case insensitive #1188
Conversation
This PR is against the
|
| @@ -24,6 +24,7 @@ jobs: | |||
| id: filter | |||
| with: | |||
| filters: "tests/config/tags.yml" | |||
| token: "" | |||
There was a problem hiding this comment.
what is this for?
There was a problem hiding this comment.
It's an attempt to reduce the amount of API calls by pulling the repo down and performing a git diff. At least that's what the docs say but I don't think it worked.
|
You need to update schema as well |
Co-authored-by: Friederike Hanssen <Friederike.hanssen@qbic.uni-tuebingen.de>
|
we cannot change params |
What could be done instead, would be to use a variable for that, which is what we where doing pre-nf-core, but with DSL2 and the way we deal with the modules in the config files, it'll quickly become a mess. |
Hmm ... How about this some place centrally: and then use do a search-replace |
I don't really like it as it obfuscates even more in the config where this tools is coming (also not even sure it'll work properly). |
adamrtalbot
changed the title
I was considering some method for checking in tools, e.g.:
But I'm not sure this is any better. Let's mark this as WIP while we try stuff out. |
I like that idea |
|
I've implemented it as |
workflows/sarek.nf
Outdated
| @@ -1215,7 +1215,7 @@ workflow SAREK { | |||
| // ANNOTATE | |||
| if (params.step == 'annotate') vcf_to_annotate = input_sample | |||
|
|
|||
| if (params.tools.split(',').contains('merge') || params.tools.split(',').contains('snpeff') || params.tools.split(',').contains('vep')) { | |||
| if (checkTools(params.tools, 'merge') || checkTools(params.tools, 'snpeff') || checkTools(params.tools, 'vep')) { | |||
There was a problem hiding this comment.
Can we have checkTools(params.tools, ['merge', 'snpeff', 'vep']) or something?
There was a problem hiding this comment.
Yes we could - I just did find + replace. Once I can get the tests working I'll tidy it up.
workflows/sarek.nf
Outdated
| @@ -1308,6 +1308,13 @@ def flowcellLaneFromFastq(path) { | |||
| return fcid | |||
| } | |||
|
|
|||
| // Check the parameters tools or skip_tools, then compare it against the provided tool | |||
| // Returns true/false based on whether 'tool' is found in 'parameter' | |||
| def checkTools(parameter, tool) { | |||
There was a problem hiding this comment.
it seems like the function checkTools could also be used for other parameters which don't have anything to do with tools (like say params.aligner or params.step), so perhaps the function might as well be called something more general like contains(if we - and nextflow - can handle that kind of "name" overloading) or your original proposal checkInParam?
Anyways, it's just a thought, and we can also just go with checkTools for now, and see which way the wind blows.
workflows/sarek.nf
Outdated
| // Check the parameters tools or skip_tools, then compare it against the provided tool | ||
| // Returns true/false based on whether 'tool' is found in 'parameter' | ||
| def checkTools(parameter, tool) { | ||
| parameter.split(',').any{ it.toLowerCase().contains(tool) } |
There was a problem hiding this comment.
How about using equals() instead of contains()? In the following example, the variable X is true and wrongly indicate that manta is among the requested tools.
tools = 'strelka,manta2'
X = tools.split(',').any{ it.toLowerCase().contains('manta') }
Y = tools.split(',').any{ it.toLowerCase().equals('manta') }
The variable Y, on the other hand, is false and correctly indicate that manta is not among the requested tools. (I know we don't have a "manta2", but you get the point.)
Also, if you go with equals() then I also suggest callingtrim() before calling equals(). The reason being that the user might add a space in the list of tools - like this:
tools = 'strelka, manta'
|
Current status, this works everywhere using the basic configuration, but fails everywhere that the config is a few levels deep because it fails to inherit the method properly. Some investigating into the scoping is required. |
- Move checkInParam to file checkInParam.nf - This allows it to be imported from everywhere - IMPORT EVERWHEEERRRREEE
Use new method
checkInParamto check if a tool is in--toolsor--skip_tools. Will centralise bug fixing etc and make everything easier, i.e. DRY.Fixes #1187
PR checklist
nf-core lint).nextflow run . -profile test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).