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

[FIX] Schema validation for empty string defaults #1100

Merged
merged 6 commits into from
Nov 16, 2021

Conversation

jpfeuffer
Copy link
Contributor

sanitise_param_default should probably always return a dict. I tried adding a test but I think adding a full extensive test schema would be better. The template that is tested, is not very complex.

@ewels
Copy link
Member

ewels commented May 31, 2021

I'm a bit lost to what this code is doing. Why would you ever set a default string of ""?

If it's related to initialising blank params, #992 may be related (everything should be initialised as null). We will be disallowing empty string defaults very soon I think.

@jpfeuffer
Copy link
Contributor Author

I agree but if you want to completely forbid this, the function should return an error instead of an object that does not fit to the general return type of the function.
Outer functions require it to return a dict.

@jpfeuffer
Copy link
Contributor Author

Or is the empty string considered to be the return type for "Failed"?

@ewels
Copy link
Member

ewels commented May 31, 2021

I agree but if you want to completely forbid this, the function should return an error instead of an object that does not fit to the general return type of the function.

Outer functions require it to return a dict.

Yup I completely agree, it definitely sounds like a bug as it stands. I was just thinking that a better fix would be to remove this code completely as part of the bigger issue. I was curious if you were deliberately setting default values like these for a reason that I hadn't thought of.

@jpfeuffer
Copy link
Contributor Author

I see! No not really, I think they were leftovers because we did not know back then that null was possible as a value.
I think it should be fine for us to use null everywhere.

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Coming back to this, I agree with the PR. This code is for fetching the values, so it's too early to be making decisions about whether those values are valid or not. That will happen later in the linting code.

@ewels ewels merged commit f58077e into nf-core:dev Nov 16, 2021
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.

2 participants