Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expected behaviour for booleans with no set default #11

Closed
awgymer opened this issue Dec 21, 2023 · 5 comments
Closed

Expected behaviour for booleans with no set default #11

awgymer opened this issue Dec 21, 2023 · 5 comments
Labels
question Further information is requested

Comments

@awgymer
Copy link
Collaborator

awgymer commented Dec 21, 2023

What is the intended behaviour for a boolean field that does not have a default in the schema but is empty/unset in the samplesheet?

I believe this may be an issue because I was under the impression that we do not support default: false in schema jsons (although maybe I am wrong here?).

Currently if there is no default and someone leaves the field unset it will result in:

in meta:
[boolean_field: null]

otherwise:
[[]]
@nvnieuwk nvnieuwk transferred this issue from nextflow-io/nf-validation Apr 23, 2024
@nvnieuwk
Copy link
Collaborator

This is a really difficult issue to tackle due to the complexity of the new JSON schema. An input can sometimes be more than one type so it's hard to define what the default should be. I'll take a look at this later and see if there maybe is some kind of a compromise to be made

@nvnieuwk nvnieuwk added the question Further information is requested label Apr 23, 2024
@nvnieuwk
Copy link
Collaborator

The easiest solution in this case would be to just set the default to false that way you specifically annotate what value should be the default instead of having an inferred default. Feel free to reopen if you disagree with this :)

@awgymer
Copy link
Collaborator Author

awgymer commented Oct 21, 2024

I am not sure what the current status of "default": false is in the schema but nf-core/tools still seems to advise against it:
https://github.com/nf-core/tools/blob/ddb58c20e757e3b2a384a5ccb8ddd2a5b4af59f9/nf_core/pipelines/schema.py#L334C1-L338C18

Perhaps @mirpedrol can shed a bit more light?

@nvnieuwk
Copy link
Collaborator

I personally don't like having inferred defaults especially for samplesheets. It just makes everything way more complicated overall IMO.

AFAIK nf-core/tools only has this for the parameters (in which case it is fine because nf-schema doesn't fill in parameter defaults). The tooling does nothing with the samplesheet schema I think

@mirpedrol
Copy link
Collaborator

Hi! Yes, the decision of removing default: false is only for parameters, this was done to avoid printing all the parameters when we print the summary of parameters that change (here some Slack conversations about this back when we made the change). We don't have tooling for the schema yet, so you can add a default there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants