Support required keyword in JSON Schema #495
Conversation
|
Great! Is this ready for review? |
|
Yes. I'm worried I've forgotten some cases but review should help spotting the things I may have missed |
|
I'm on vacation, will take a look as soon as I can! |
rlouf
force-pushed
the
support_required_to_regex
branch
from
b03e043 to
4777ff2
Compare
| properties = instance["properties"] | ||
| required_properties = instance.get("required", []) | ||
| is_required = [item in required_properties for item in properties] | ||
| # If at least one property is required, we include the one in the lastest position |
There was a problem hiding this comment.
Is this changing the order of the fields as they were specified in the JSON schema?
There was a problem hiding this comment.
# We need to make sure that the last item generated does not end with a comma and thus end up with invalid JSON.
# When at least one property is required, we select the last in the list and include it at the last position without a comma. We then prepend every prior (optional or required) property with a comma.There was a problem hiding this comment.
Why not do the same as below, actually? This may simplify the code by considering both cases at once?
There was a problem hiding this comment.
This is not changing the order of the fields.
An issue with When at least one property is required, we select the last in the list and include it at the last position without a comma is that it would change the order of the fields if the last property is an optional one.
There was a problem hiding this comment.
I can't see a way of easily combining both as the case in which there are no required properties creates as many subregexes as there are properties and connect them with | while the case with at least one required property does not need that.
rlouf
force-pushed
the
support_required_to_regex
branch
from
4777ff2 to
3267adb
Compare
|
We need to fix that merge conflict before merging |
I can't see the conflict. I'm told my branch is up to date when trying to rebase in git and I have "Able to merge" on Github on my forked repository |
|
Thank you! |
The aim of this PR is to solve issue #419