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

Introduces Nullable validator #81

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

linnal
Copy link

@linnal linnal commented Nov 1, 2022

This is another "refactored" version of this PR
Here I tried to follow the current logic and add a Validator.
The current problem I encountered is that validators get executed only if the property is set.
By adding a Nullable property, we need to make sure that Types does not complain anymore about nil values. This goes against the current logic and current tests suit, which assumes that nil should not be allowed a valid value for the defined types.
So, there is a decision to be made here:

  1. Allow Types to accept nil and let nullable to do the check if nullable is set => By default nil is accepted as a value, especially if nullable is not set
  2. Keep the current logic of not allowing nil as a valid value, but add somehow nullable: false by default
  • If we go with step 1 there are lots of tests that need changing

Why do we want to allow Types to have nil value? Because validators are independent, and if nullable is set to true, we do not want Types to complain related to nil values.

Maybe there is another option I'm not considering, let's discuss further :)

@linnal linnal marked this pull request as draft November 1, 2022 10:33
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.

1 participant