balena-io-modules / jellyschema

JellySchema - data validation, UI form generation
Apache License 2.0
4 stars 2 forks source link

Add support for enums on integers #50

Closed cyplo closed 5 years ago

cyplo commented 5 years ago

As per #15 and #49 - on master we only support enum keyword on string type - this PR makes the enum support be more generic and applies it to the integer type as well.

Not adding support for enum on any other base type for now - let's see what is needed. E.g. not sure if we need/want support for enums on boolean etc.

Extracted out enum module and made EnumerationValue store Value instead of String to be able to abstract over any type that can be enumerated.

Fixes #49

cyplo commented 5 years ago

Please, don't guess and read the specification. enum is in the Validation Keywords for Any Instance Type chapter. Any means any type, not just integers or strings.

Sure thing @zrzka - the spec is not fully supported yet as per #15 - is missing some things. My thinking was to add them incrementally, with the priority coming with the usage from the ecosystem, as then it's easier for me to construct the test cases. For example - the enums on integers I needed to showcase validation troubles I'm having when displaying the forms. But for e.g. enums on booleans - I was not able how to construct a test case that would present a real-life scenario.

What would you suggest here for this PR ? Add a support for all other types now in same PR ? Separate PR ?

zrzka commented 5 years ago

Okay, approved. I missed the on integers in the title.

More general comment:

I was not able how to construct a test case that would present a real-life scenario

Real-life can differ when one compares it against the spec. Schema is generic, and sometimes, it can look like that it doesn't make sense, ... But this doesn't mean we shouldn't do it.

Anyway, don't do it now. We have to think about all these things over Christmas, decide what to do in January and then do it. So, use your time for the playground & rendition, cdsl if something's broken, but don't spend so much time on cdsl, because there's a possibility of major overhaul.

cyplo commented 5 years ago

cool, thanks so much :) back to UI land :)