-
Notifications
You must be signed in to change notification settings - Fork 9k
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(config): define missing default options #9949
Conversation
@glowcloud didn't we forget to assing proper typecasting to these new config options? |
We were skipping typecasting for functions before so we need to add it for all of them then. |
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.
Looks good, but we need to address the code review comments before this can go in.
Let's go for it. Some reasoning: Let's assume there would nullable-function typecaster. What purpose would it serve? |
I also noticed that we're missing a few type casters for some default options, which are not defined in our documentation:
There's also one default value that I don't see the usage for: |
it's in defaults, but doesn't have type caster. Should be
missing from defaults, doesn't have type caster. Should be
defined in defaults, don't have type caster. Should be both |
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.
Looks good. Nicely done!
Refs #9945
Choices for the default values explained in: #9945 (comment)