Skip to content

Update sourmash sketch to remove hardcoded optional argument - #12999

Merged
SPPearce merged 36 commits into
nf-core:masterfrom
jfy133:sourmash-sketch-remove-optional-parameter
Sep 20, 2026
Merged

SPPearce merged 36 commits into
nf-core:masterfrom
jfy133:sourmash-sketch-remove-optional-parameter

Conversation

@jfy133

@jfy133 jfy133 commented Sep 19, 2026

Copy link
Copy Markdown
Member

SOURMASH_SKETCH included an actually optional argument --merge which was having unintended 'hidden' behaviours in some cases.

This PR moves the parameter to an input boolean, and updates all tests to keep the original behaviour using the new system.

jfy133 and others added 30 commits January 23, 2023 15:03
@jfy133

jfy133 commented Sep 19, 2026

Copy link
Copy Markdown
Member Author

@nf-core-bot fix linting

@jfy133

jfy133 commented Sep 19, 2026

Copy link
Copy Markdown
Member Author

Ugh failing tests... I bet because of row ordering 🙄

@SPPearce
SPPearce added this pull request to the merge queue Sep 20, 2026
Merged via the queue into nf-core:master with commit 6eaecd6 Sep 20, 2026
69 checks passed
Specify whether to merge the signatures of all input files into a single signature file.
Used e.g. for paired-end FASTQ inputs, or when there are multiple FASTAs for a single genome (such as chromosomes or contigs).
Signature name will be set to the processes' '${prefix}'.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being late to the party @jfy133, why this extra input rather than relying on ext.args set by a user?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the original implementation used prefix as the default name (which is a reasonable assumption in this case), but prefix is not accessible via modules.conf.

@haris18s

Copy link
Copy Markdown
Contributor

I was about to push same PR for merge and i just saw yours, i guess no need anymore :)

Schmytzi pushed a commit to Schmytzi/nf-core-modules that referenced this pull request Sep 22, 2026
…#12999)

* Bump das_tool versions

* Roll back scaffolds2binversion as this is a legacy

* Correct build container

* Missing quote

* Fix bug in conda scope check condition for antismash download databases

* Update main.nf

* Update Kaiju to also emit the downstream required nodes file

* Include names file as well

* Update documentation

* update all Kaiju modules to topics

* Remove debug thing from sourmash

* Reset KMCP files

* [automated] Fix code linting

* Move previous implicit (but optional) argument to input channel option

* Update taxannotate tests to include missing arguments in upstream module in test

* [automated] Fix code linting

* Updates snapshot to account for variability

* Update merge test due to random selection of main file name

* Relax further

---------

Co-authored-by: nf-core-bot <core@nf-co.re>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants