Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Mixed support #2409
base: master
Are you sure you want to change the base?
Mixed support #2409
Changes from 36 commits
f8c2caf
c078710
ece92ac
bd28667
a62efb4
4bc6b73
4716a43
807546f
25300db
81c81db
3a4f2b9
6b5dca8
2f96aef
cc44091
d460e6c
1f24b21
d716953
498cbf1
72e17d0
89bfb0b
ef24610
492e5fd
434a1f9
532fa9f
22760af
8a1b523
1d51ee8
770531a
6ead876
b7460ab
27c11fe
318950e
3a6f240
e3c8b3c
40dc34f
a7fa97b
9111316
0a6047f
1c146bd
fd7efb4
0e1577b
6b863d9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 feel this is plastering over bugs. If a schema can't be resolved then this means the handed over
schema
,uischema
combination was wrong. It's better to fail early than falling back to the unresolved 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.
I don't like the cast to
any
here. If we really expectResolve.schema
to being able to return booleans, then its typing should indicate that.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 don't like this. Up until now, the
schema
returned from the mappers was always a sub-schema of the overall schema. Not only does this not return an actual sub-schema, it also produces a newresolvedSchema
whenever it is executed, which will breakmemo
barriers.I'm all for being able to handle
boolean
(sub-)schemas but the change should be less intrusive. I would like to keep the original prerequisite for now: Always return a sub-schema of the original schema. Once we break that, for example to support schema overrides, this becomes much more complicated because of the object identities.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.
What is the use case of this? Who hands over segments to resolve if the schema is just a boolean already anyway?
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.
that is to resolve the case where the value of the last path segment is equals to true. For example items: true and you want to bind that items then you need to resolve the jsonschema which can be boolean but in the code we ignore that this can be ever the case.
Also the next check for isEmpty if we pass true/false values which are valid JSON schema will return an undefined when resolved instead of boolean
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.
The case is already handled via
If having a boolean
schema
breaks the checks before that, then we should fix those checks instead of hard coding the exception at the top.I guess the typing does not properly reflect that the
schema
could be a boolean. So we should adjust the typing and the checks to handle the typing.