-
Notifications
You must be signed in to change notification settings - Fork 399
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
Support required keyword in JSON Schema #495
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! |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this changing the order of the fields as they were specified in the JSON schema?
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.
# 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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