https://nf-co.re logo
Join Slack
Powered by
Tweak to samtools/bgzip to make it accept more for...
# request-review
i
Tweak to samtools/bgzip to make it accept more formats: https://github.com/nf-core/modules/pull/11368/
Passing CI now. I know this module will likely be deprecated in the future, but can I get a review for now?
s
Sorry, but you can't use 
prefix2
as a replacement for 
suffix
. It makes perfect sense to me to allow ext.suffix, but I was outvoted
party this 1
m
yeah, Let's not use prefix2 for suffix
i
What's the correct way to handle this?
m
I assume you changed suffix for prefix2 because of some linting errors
i
Yes, suffix is certainly not allowed in the current version
m
I would say let's keep suffix and force merge before anyone else notice
I wanted suffix too and was outvoted with Simon
(I said suffix too, not suffix2)
i
I found the voting, you two were the only supporters
Although it's true that only a few modules can output arbitrary formats
s
🤷 it didn't have to be arbitrary, it was being used for sam/bam/cram, or vcf/bcf/vcf.gz
Feel free to go and #C07KND2JP54
i
Changed to the Mahesh version, is it okay like this?
I feel like the philosophy of value input vs. ext is not too mature, unless I'm misunderstanding something
Although my gut feeling is that Mahesh was right about the use case you're describing, since in that case it affects the logic of the tool
But then again so does ext.args
s
🤷, it really depends on the tool
j
But then again so does ext.args
I'm a member of the "args and prefix should be process inputs" club 🫣
i
I just wanted to update a module, opened an old can of worms instead
🤣 2
m
Welcome to nf-core
🤣 1
You're a maintainer already, right?
Anyone that open such a can of worm deserve to be one
i
In any case, tweaks applied and CI passes
You're a maintainer already, right?
Indeed
m
Find bigger issues and proper some solutions, and you can probably raise to higher levels
i
I'm not fixing any big issues before record types arrive
🤣 1
@Simon Pearce @Jim Downie can you check if it's okay now?
All comments solved, just waiting for @Simon Pearce to unblock the PR
j
I have dismissed @Simon Pearce's review
🤣 1
m
Bold
i
If he doesn't complain before 10am I'm merging it
😬 1
s
Any particular 10am?
Always feel free to dismiss stale reviews
i
My 10am
It's in queue now, hopefully it's all fine
🙌 1
s
You have 2 approvals, you don't need mine too
I just put the Needs Changing as a block to ensure it didn't get approved with a sneaky 
prefix2
i
Merged now, thanks everyone
❤️ 2
2 Views
Open in Slack
PreviousNext
Powered by