sarek icon indicating copy to clipboard operation
sarek copied to clipboard

Published 20 hours ago • verified publisher icon nf-core

[WIP] --tools is now case insensitive

Open adamrtalbot opened this issue 2 years ago • 12 comments

Use new method checkInParam to check if a tool is in --tools or --skip_tools. Will centralise bug fixing etc and make everything easier, i.e. DRY.

Fixes #1187

PR checklist

  • [ ] This comment contains a description of changes (with reason).
  • [ ] If you've fixed a bug or added code that should be tested, add tests!
  • [ ] If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • [ ] If necessary, also make a PR on the nf-core/sarek branch on the nf-core/test-datasets repository.
  • [ ] Make sure your code lints (nf-core lint).
  • [ ] Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • [ ] Usage Documentation in docs/usage.md is updated.
  • [ ] Output Documentation in docs/output.md is updated.
  • [ ] CHANGELOG.md is updated.
  • [ ] README.md is updated (including new tool citations and authors/contributors).

adamrtalbot avatar Aug 18 '23 13:08 adamrtalbot

This PR is against the master branch :x:

  • Do not close this PR
  • Click Edit and change the base to dev
  • This CI test will remain failed until you push a new commit

Hi @adamrtalbot,

It looks like this pull-request is has been made against the adamrtalbot/sarek 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 adamrtalbot/sarek 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!

github-actions[bot] avatar Aug 18 '23 13:08 github-actions[bot]

You need to update schema as well

maxulysse avatar Aug 19 '23 07:08 maxulysse

nf-core lint overall result: Passed :white_check_mark: :warning:

Posted for pipeline commit 81462b2

+| ✅ 140 tests passed       |+
#| ❔   9 tests were ignored |#
!| ❗   4 tests had warnings |!

:heavy_exclamation_mark: Test warnings:

:grey_question: Tests ignored:

:white_check_mark: Tests passed:

Run details

  • nf-core/tools version 2.9
  • Run at 2023-09-07 11:33:51

github-actions[bot] avatar Aug 19 '23 10:08 github-actions[bot]

How about making the change just one place centrally? Something like

params.tools = params.tools.toLowerCase()

in sarek.nf? Then there should be no danger of me forgetting to add .toLowerCase() when doing a contains() on, say params.tools.split(',')`:-)

we cannot change params

maxulysse avatar Aug 21 '23 09:08 maxulysse

How about making the change just one place centrally? Something like params.tools = params.tools.toLowerCase() in sarek.nf? Then there should be no danger of me forgetting to add .toLowerCase() when doing a contains() on, say params.tools.split(',')`:-)

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.

maxulysse avatar Aug 21 '23 09:08 maxulysse

How about making the change just one place centrally? Something like params.tools = params.tools.toLowerCase() in sarek.nf? Then there should be no danger of me forgetting to add .toLowerCase() when doing a contains() on, say params.tools.split(',')`:-)

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:

tools = params.tools.split(',').toLowerCase()

and then use do a search-replace params.tools.split(',').toLowerCase() all over this PR?

asp8200 avatar Aug 21 '23 09:08 asp8200

How about making the change just one place centrally? Something like params.tools = params.tools.toLowerCase() in sarek.nf? Then there should be no danger of me forgetting to add .toLowerCase() when doing a contains() on, say params.tools.split(',')`:-)

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:

tools = params.tools.split(',').toLowerCase()

and then use do a search-replace params.tools.split(',').toLowerCase() all over this PR?

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).

maxulysse avatar Aug 21 '23 09:08 maxulysse

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).

I was considering some method for checking in tools, e.g.:

checkInParam(params.tools, "mutect2")

But I'm not sure this is any better. Let's mark this as WIP while we try stuff out.

adamrtalbot avatar Aug 21 '23 09:08 adamrtalbot

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).

I was considering some method for checking in tools, e.g.:

checkInParam(params.tools, "mutect2")

But I'm not sure this is any better. Let's mark this as WIP while we try stuff out.

I like that idea

maxulysse avatar Aug 21 '23 09:08 maxulysse

I've implemented it as checkTools method to be reused in configuration. Let's see if it works 🤞

adamrtalbot avatar Aug 22 '23 16:08 adamrtalbot

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.

adamrtalbot avatar Aug 29 '23 16:08 adamrtalbot

@adamrtalbot As part of spring cleaning 🌻 I would ask if you will still continue on this PR (with all the merge conflicts)?

famosab avatar Mar 12 '25 13:03 famosab