-
Notifications
You must be signed in to change notification settings - Fork 20
Monochrome logs is option in functions rather than global parameter #101
Monochrome logs is option in functions rather than global parameter #101
Conversation
Rather than reading it from global params.monochrome_logs, monochrome_logs is an argument supplied to the function. This means a pipeline developer can configure it more precisely.
@nvnieuwk a possible option here would be to default it to params.monochrome_logs, false if params.monochrome_logs does not exist? Equivalent to:
This would make it backwards compatible, but also make it complicated for developers (unexpected behaviour). |
I'm all for backwards compatibility! Thanks for the contribution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this but my main concern is now that we have two positional arguments to the function. This means you have to specify the schema if you want to specify the monochrome_logs
argument too.
You can have a look at the .fromSamplesheet()
factory for an example on how to handle this:
nf-validation/plugins/nf-validation/src/main/nextflow/validation/SchemaValidator.groovy
Line 150 in 7ca22a4
public DataflowWriteChannel fromSamplesheet( |
(which uses the same method as core nextflow functions with optional arguments)
Thanks, not sure I understand which bit I'm supposed to be looking at though? Should I add the arguments as the |
Yes and some code that looks if the option is supplied like here: nf-validation/plugins/nf-validation/src/main/nextflow/validation/SchemaValidator.groovy Lines 158 to 159 in 7ca22a4
|
7602082
to
cab2a40
Compare
I've done it now but I'm not sure it's reading from the optional args correctly, can you check for me? [edit] hold on, git faff... |
OK I've pushed the changes but it doesn't work because of positional args stuff. Don't really have any more time to look at it I'm afraid. |
Oh! I seem to have fixed it. Need to update docs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you I will look at it in detail when I got some more time, it does look very good at first glance though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
My only comment is that if this is now a parameter from the plugin, we should refactor to monochromeLogs
to follow the Nextflow convention.
edit: could we refactor the argument name in the options
map, but allow both parameters params.monochrome_logs
and params.monochromeLogs
when retrieving the global parameter? The nf-core template is using params.monochrome_logs
in other functions, such as creating the logo or formating emails. But other non-nf-core pipelines will use params.monochromeLogs
as nextflow params are in camelCase
I agree. So
Do you want it in this PR or a separate one? |
Can be added to this PR :) Should be a small enough change |
If someone sets |
Current priority:
|
sounds good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @adamrtalbot
I don't have permission to merge but this is good to go from my end 👍 |
Oh, unless you want me to extend the tests. I haven't looked too deeply into that yet. |
Oh yes more tests won't hurt anybody 💯 |
…into monochrome_logs_is_option
This changes monochrome logs to being an option in paramsSummaryLog(), paramsSummaryMap() and paramsHelp().
This makes the structure more flexible for pipeline developers who can now use params.monochrome_logs or handle it with their own tooling.
Fixes #99