-
Notifications
You must be signed in to change notification settings - Fork 639
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
Handle const
like enum
with only one option
#1446
Conversation
By definition `const: xxx` is equivalent to `enum: [xxx]` : https://json-schema.org/draft/2020-12/json-schema-validation#name-const , so e.g. for "abc": { "type": "string", "const": "ABC" }, "def": { "type": "string", "enum": ["DEF"] } `abc` and `def` fields should be handled in the same way via select with only one item. Fixes json-editor#1428
There are a lot of changes in the test itself, see https://github.com/json-editor/json-editor/pull/1446/files#diff-a58cc456b73da808d706059b06e26d464924feb9ee3f469e44fed59923a30c02 Were the tests wrong? CC: @germanbisurgi |
@schmunk42, thanks for feedback. Yes, those oneOf-2 and anyOf-2 tests needed to be changed, because those tests work on schemas with |
( here is the schema used in oneof-2 test: https://github.com/json-editor/json-editor/blob/67f29f49/tests/pages/oneof-2.html#L23-L67 ) |
@schmunk42, @germanbisurgi, do you think you could review this patch, or is there something wrong with it? Offhand I would say this patch is much simpler compared to e.g. $ref resolver fixes, and it should be relatively straightforward to review. Thanks beforehand for feedback, |
@navytux Sorry for that. We'll try to find time next week. @germanbisurgi Please have a look, especially the changed tests ... but we might have checked a bug :) |
Thanks, @schmunk42. |
Hello @schmunk42 and @germanbisurgi. Would you please have a look at this patch? Basically it adjusts the editor to do what JSON-schema specification literally requires conformant implementations to do:
( https://json-schema.org/draft/2020-12/json-schema-validation#name-const ) So from that point of view this patch and adjustments to the tests should be obviously correct. This patch was posted here back in December, but I see that in January another test was added that expects that it is possible to arbitrarily edit fields with which, from the point of view that "const is equivalent to enum with single value" is also incorrect and should be reworked, since e.g. for a On my side I prefer to get at least some high-level feedback first, before delving into any code rework, if at all. I also understand that our pre-history on #1385 might be an issue. When I started my journey into JSON-schema world I was complete newbie and did not realize that in my case the editor will get already resolved schema and so there won't be the need to handle Regarding changes themselves I would say that hereby So I would appreciate if Thanks beforehand for feedback, |
Hi @navytux, i am currently reviewing this your PR. The implementation looks good to me. I have to take a deeper look at the tests changes. Just wanted to add feedback at the moment. |
Thanks, @germanbisurgi. |
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 will bring a minor breaking change in that the default keyword will be ognored, for example:
"string": {
"title": "Some string",
"type": "string",
"const": "test",
"default": "not in const"
}
"not in const" will be ignored and never shown.
Otherwise it looks good to me. The master branch should be merged into this PR before we merge it.
@schmunk42 Do you think it would be a good idea to add this feature behind a option like, "const_to_enum"?
@navytux This is ready to merge this, but there is a small conflict. could you resolve it in your branch please? |
fix lint errors
fix linting errors
So, i resolve the merge conflict and fixed some linting errors introduced by myself (no linting in GitHub editor) but it has still test not passing. @navytux could you check on this and update code? |
@germanbisurgi thanks for feedback and updating the branch, and @schmunk42 thanks for approving this PR. I'm currently travelling and will try to have a look to finalize this PR in several days. Sorry for the delay, and thanks, once again, |
Hello @navytux, upon further exploration of the 'const' topic, we've decided to pursue an alternative approach to ensure better backward compatibility for this feature, while retaining the integrity of the existing tests. You can review the PR here: #1555. Your input on this would be greatly appreciated. |
@germanbisurgi, thanks for doing the alternative patch. #1555 works for me. |
By definition
const: xxx
is equivalent toenum: [xxx]
: https://json-schema.org/draft/2020-12/json-schema-validation#name-const , so e.g. forabc
anddef
fields should be handled in the same way via select with only one item.Fixes #1428