-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: removed state as required property. Allowed variants of boolean flag to be null #81
Conversation
… flag to be null. Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
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.
This looks right to me, but I think this was mostly @beeme1mr 's idea, so I'd like his sign off as well.
I didn't get a chance to properly review this today. I'll look into it tomorrow. |
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.
I tested it out today. It works but the UX isn't great at the moment. For example, adding a variant
property will autocomplete the boolean variant in VS Code. It would also be nice if the default variant could be an enum contain true
and false
when the variant property was omitted or matched { "true": true, "false", false }
.
I would love to figure out a way to autocomplete the default variant based on the variant keys but I don't think that's possible with a JSON schema.
The goal should be to make the most common use case as simple and safe as possible.
This is because the boolean type is the only one with default variants. We can omit the default from the json schema and infer it in flagd if we'd rather the autocomplete created an empty object.
I've hunted around but not been able to find any indication that this is possible within the schema (e.g. stack overflow answer declaring it's not possible). |
I think the autocomplete experience in a particular tool isn't a good basis for deciding how we build our schema. The changes here seem to match the issue exactly. |
@beeme1mr This PR has gone stale. We should follow-up and either close or go ahead with this |
This PR
Related Issues
Fixes #80
Notes
Follow-up Tasks
How to test