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(config): define missing default options #9949

Merged
merged 4 commits into from
May 21, 2024
Merged

fix(config): define missing default options #9949

merged 4 commits into from
May 21, 2024

Conversation

glowcloud
Copy link
Contributor

Refs #9945

Choices for the default values explained in: #9945 (comment)

@char0n
Copy link
Member

char0n commented May 21, 2024

@glowcloud didn't we forget to assing proper typecasting to these new config options?

@glowcloud
Copy link
Contributor Author

@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.

Copy link
Member

@char0n char0n left a 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.

@char0n
Copy link
Member

char0n commented May 21, 2024

We were skipping typecasting for functions before so we need to add it for all of them then.

Let's go for it.

Some reasoning: Let's assume there would nullable-function typecaster. What purpose would it serve?
IMHO the same as nullable-string typescaser. We either get function we can execute or null.
So IMHO it make sense to add nullable-function as it makes what the option can or cannot be explicit.

@glowcloud
Copy link
Contributor Author

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: custom

@char0n
Copy link
Member

char0n commented May 21, 2024

configs

it's in defaults, but doesn't have type caster. Should be object typecaster.

initialState

missing from defaults, doesn't have type caster. Should be object typecaster.

components, fn

defined in defaults, don't have type caster. Should be both object typecaster.

Copy link
Member

@char0n char0n left a 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants