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

#353 correctly instantiate boolean properties and improve spectral rules #500

Merged
merged 4 commits into from
Oct 18, 2024

Conversation

willosborne
Copy link
Member

Fixes #353. Boolean properties will be output as strings with the value {{ BOOLEAN_[property name] }}
We can't output false by default because this is a valid option and thus we can't detect it.
I wanted to keep the output valid JSON, so decided to spit out a string.

Also adds

  • spectral rule to report boolean placeholders
  • spectral rule to warn users that booleans are not recommended if detected in a pattern

@aidanm3341
Copy link
Member

I understand why you've done it this way, spitting out a string instead of a boolean, but doesn't this mean that the generated JSON isn't valid in reference to the pattern? If the pattern requires a boolean, and the validator (or any other json schema tool) sees a string, I would expect it to throw an error?

This would mean that the generate command no longer creates a valid instantiation. Do you agree or am I missing something? I also don't have an alternative, but I just wanted to point that out

@willosborne
Copy link
Member Author

willosborne commented Oct 18, 2024

You're right, but I can't think of an alternative. We can't output either true or false as a placeholder, we would be unable to distinguish this from a const field and it would be a very dangerous move IMO.

Also note that array generation is the same. Currently we spit out an array with a single string element containing a placeholder - if you've asked for an array of integers this won't validate.

Side note - you do get quite a good error message from the json schema validation though! Perhaps a change to always validate post-generation could help improve the user experience here?

Regardless, from I can think of the options are either:

  • output something that isn't valid JSON at all, such as a blank field
  • output something that isn't valid against the pattern, such as a string
  • prompt for the value of a boolean property at generation time - this is something we were thinking about for all placeholders, to help automation of filling in patterns. but i don't think it makes sense to do here
  • simply fail if we see a boolean property - formally drop support for boolean properties

@rocketstack-matt @Thels interested in your thoughts here.

@Thels
Copy link
Member

Thels commented Oct 18, 2024

Please take my input with some grains of salt, I'm still new to the space! ( how long do I get to play this card? :P )

Creating a placeholder like this makes it obvious that there is input required from the user in some form to "finish" what they're generating. I much prefer it to the idea of generating something that ends up being incorrect for a "hidden" reason.

If we think about how this feeds into things like Extensions / CLI - both the extension and the validation CLI will highlight this placeholder as something that needs addressed.

@rocketstack-matt
Copy link
Member

Please take my input with some grains of salt, I'm still new to the space! ( how long do I get to play this card? :P )

Creating a placeholder like this makes it obvious that there is input required from the user in some form to "finish" what they're generating. I much prefer it to the idea of generating something that ends up being incorrect for a "hidden" reason.

If we think about how this feeds into things like Extensions / CLI - both the extension and the validation CLI will highlight this placeholder as something that needs addressed.

I agree, the purpose of placeholders is to replace them and we've had to build the validate command to warn about them because in most cases they're valid. I suspect given the choice we would prefer all the tools to say they needed to change.

@rocketstack-matt rocketstack-matt merged commit 82d375b into finos:main Oct 18, 2024
4 checks passed
@willosborne willosborne deleted the boolean-props branch October 21, 2024 09:05
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.

Generate does not output placeholders for boolean properties
4 participants