Combine const/enum with type error messages#169
Combine const/enum with type error messages#169jdesrosiers merged 6 commits intohyperjump-io:mainfrom
Conversation
jdesrosiers
left a comment
There was a problem hiding this comment.
It didn't occur to be that you'd have to duplicate the logic for determining the type in the constEnum handler. That tells me that my original thought that this should be one combined typeConstEnum handler is probably better.
Now, I've combined the handlers into a single |
ddb5384 to
330969b
Compare
jdesrosiers
left a comment
There was a problem hiding this comment.
This looks way more complicated than it should be. Can you see if it can be simplified? Also, I'd like to see a few more tests. This is a complicated feature especially when it comes to schema locations. Let's see if we can cover some more edge cases.
jdesrosiers
left a comment
There was a problem hiding this comment.
The added tests look good. I left a suggestion to try to help simplify the filtering approach.
| const keyword = await getSchema(schemaLocation); | ||
| const keywordJson = new Set([jsonStringify(/** @type Json */ (Schema.value(keyword)))]); | ||
|
|
||
| allowedJson = allowedJson?.intersection(keywordJson) ?? keywordJson; |
There was a problem hiding this comment.
If we do the filtering as we go, we can skip that whole large block of code dedicated to filtering. I think this code is more clear and concise than filtering after.
| const keyword = await getSchema(schemaLocation); | |
| const keywordJson = new Set([jsonStringify(/** @type Json */ (Schema.value(keyword)))]); | |
| allowedJson = allowedJson?.intersection(keywordJson) ?? keywordJson; | |
| const keyword = await getSchema(schemaLocation); | |
| const keywordJson = new Set(); | |
| if (allowedTypes.has(Schema.typeOf(keyword))) { | |
| keywordJson.add(jsonStringify(Schema.value(keyword))); | |
| } |
There was a problem hiding this comment.
I refactored the const/enum collection to filter by type inline, as suggested.
jdesrosiers
left a comment
There was a problem hiding this comment.
Yeah, that looks much better. I left a few more note for things that could be improved.
|
cleanup improvement done..
|
Fixes #164
When validation fails with both
typeandconst/enumkeywords, this PR shows only the more specific const/enum error with type-aware value filtering.Changes:
constEnumhandler to filter allowed values by type constraintstypehandler to suppress errors when const/enum is present