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

Add remaining schema renderers #5534

Merged
merged 2 commits into from
Oct 21, 2022

Conversation

antgamdia
Copy link
Contributor

@antgamdia antgamdia commented Oct 20, 2022

Description of the change

In #5436 we mentioned that several fields were still pending, namely

  • 1st level: object (at least, as array of tuples <string, any> for defining each property.
  • Array level: enum, arrays, objects

WRT the top-level objects, it is not an actual problem: if they have properties, they will get rendered as any other nested param more. It is only the case when it lacks the properties fields that we need to handle.

  • Solution: allow any serialized object like {"foo": 1234} and add more validation messages.

WRT the array-level properties:

  • Solution for enum: use the same approach as in the top-level enum.
  • Solution for arrays and objects: use the same approach as in the top-level objects: render a text field but enforce validation.

Additionally, the array now enforces maxItems and minItems

Benefits

Every json schema datatype is now supported.

Possible drawbacks

N/A

Applicable issues

Additional information

Note
This PR is part of a series of PRs aimed at closing this milestone. I have split the changes to ease the review process, but as there are many interrelated changes, the tests will be performed in a separate PR (on top of the branch containing all the changes).
PR 5 out of 6

Top-level objects:

image

Array<enum>:

image

Array<array>:

image

Array<object>:

image

YAML output

image

Array max/min items

Min items:
image

Max items:
image

Copy link
Collaborator

@castelblanque castelblanque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for the improvement. Table looks really nice!
Shouldn't we add tests to avoid regressions? I mean unit tests here, the upcoming tests will be the E2E?

@antgamdia
Copy link
Contributor Author

Great, thanks for the improvement. Table looks really nice!

Thanks! Definitely, editing the schema is a great addition for users wanting to create forms/schemas for their packages.

Shouldn't we add tests to avoid regressions? I mean unit tests here, the upcoming tests will be the E2E?

Absolutely! The point is that, given that the changes are pretty coupled, I just wanted to split the code to make the review easier, but I haven't actually tested each PR separately (meaning I for sure have forgotten to add a change that will be in another PR).
My approach is to add the e2e tests in a separate PR (which I'll create soon). I will merge everything but the first PR and then I'll create the one with the tests on top of it.

@antgamdia antgamdia merged commit b94ee74 into 4-5520-fixNumValidation Oct 21, 2022
@antgamdia antgamdia deleted the 5-5436-addSchemaRenderers branch October 21, 2022 16:43
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.

3 participants