Add comment about setting a default prefix that isn't just meta.id #2608
Conversation
✅ Deploy Preview for nf-core-main-site ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
failing playwright tests can be ignored for now |
|
|
||
| ```nextflow | ||
| script: | ||
| if ("$bam" == "${prefix}.bam") error "Input and output names are the same, set prefix in module configuration to disambiguate!" | ||
| ``` | ||
|
|
||
| - If the input and output files are likely to have the same name, then an appropriate default prefix may be set, for example: |
There was a problem hiding this comment.
| - If the input and output files are likely to have the same name, then an appropriate default prefix may be set, for example: | |
| - If the input and output files are likely to have the same name, then an appropriate default prefix MAY be set, for example: |
I feel this should be left for the developer to decide how the resulting file should be called. The error should make them aware of this.
There was a problem hiding this comment.
Setting the default prefix does give the control to the developer, as they can overwrite it in the modules.config. The problem is when it's hard-coded into the output path.
I also added the -C bash flag to the shell directive in the template so that should also prevent accidental clobbering.
There was a problem hiding this comment.
The problem is when it's hard-coded into the output path.
I don't think I follow... isn't the suggestion here to technically embed a hardcoded string?
There was a problem hiding this comment.
No, the example clearly shows prefix having a default of "${meta.id}_sorted". If I wanted something different, I just update in the config
ext.prefix = { "${meta.id}_mysorted" }
The command should still look like:
mycommand --input $file > ${prefix}.out
but the file goes from <meta.id>_sorted.out to <meta.id>_mysorted.out
There was a problem hiding this comment.
Yes, it is setting a default, but can be entirely overwritten in the usual way
There was a problem hiding this comment.
Disagree on both points (1. it should, pipeline devs should be very aware of the output files, 2. I don't think we should sacrifice flexibility just to avoid a small config file).
Bbut like I said - 'small mound' 😆 Maybe ask for one more opinion and you can merge
I mean, we could say that pipeline devs should make each module from scratch because that would be purer. I don't understand your objection here. This sacrifices no flexibility whatsoever, it defines a default prefix that can then be overwritten in exactly the usual way.
There was a problem hiding this comment.
I also don't understand the objections. Is there any way you can clarify in a toy example of what the objection is?
There was a problem hiding this comment.
I follow James here. IMO it's better to have a common way of doing things in modules so you don't have to check how it's done in every module. If I know every module has the meta.id as default, then I don't have to check the default every time. Adding different defaults could become confusing for some developers. But also not a hill I'm willing to die on :)
There was a problem hiding this comment.
My experience with using nf-core modules, when written by someone else is usually that I'll have to check the code anyway to know what the output channels are named at the least, and what inputs I'm expecting. If I then see the
if ( file.name == "${prefix}.ext" ){
I'll know there's a possibility for a filename collision. However, this is where I would expect the module to have a sensible default not to have filename collision if I just plug and play. More often than not though, the current state is that once I use it, I'll discover the default is not actually sensible and have to modify my modules.config to deal with it. This to me is a time waster. If we did use "sensible defaults", rather than "meta.id", then I'll see when I inspect the module to change prefix if I wanted it differently, but I'd rather not assume that ${prefix}.ext will result in a filename collision if I don't set my own prefix anyway. That's at least my experience, which is why I'm for this update.
There was a problem hiding this comment.
Maybe we should move this discussion to the next maintainers meeting? This seems like a topic that could fit very well in the meetings
|
What is the state of this docs update, @SPPearce ? |
ah, yes, remember now. let's try to bring it up in the next meeting then. |
|
Sorry for being difficult 😁 let's talk about it in the next meeting indeed :) |
No description provided.