-
-
Notifications
You must be signed in to change notification settings - Fork 4
Fix: Add support for format keyword across all JSON Schema dialects #95
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
Conversation
Signed-off-by: Diya <[email protected]>
arpitkuriyal
left a comment
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.
Please ensure there are no linting errors in your PR by running npm run lint before creating a PR.
I guess, it’s not necessary to test every dialect, one test case is sufficient.
sure @arpitkuriyal , i forgot to run lint test at the end , but i have fixed it now. |
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 current tests contain a lot of repetition. If we still want to verify behavior across all dialects, we can replace the duplicated tests with a single parameterized test like this
const dialects = [
{ name: "draft-2020-12", schema: "https://json-schema.org/draft/2020-12/schema" },
{ name: "draft-2019-09", schema: "https://json-schema.org/draft/2019-09/schema" },
{ name: "draft-07", schema: "http://json-schema.org/draft-07/schema" },
{ name: "draft-06", schema: "http://json-schema.org/draft-06/schema" },
{ name: "draft-04", schema: "http://json-schema.org/draft-04/schema" }
];
for (const { name, schema } of dialects) {
test(`format: email (${name})`, async () => {
registerSchema({
$schema: schema,
format: "email"
}, schemaUri);
const instance = "not-an-email";
const output = {
valid: false,
errors: [
{
absoluteKeywordLocation: "https://example.com/main#/format",
instanceLocation: "#"
}
]
};
const result = await betterJsonSchemaErrors(output, schemaUri, instance);
expect(result.errors).to.eql([
{
schemaLocation: "https://example.com/main#/format",
instanceLocation: "#",
message: localization.getFormatErrorMessage("email")
}
]);
});
}
hey @arpitkuriyal i think i have addressed the situation, thanks a lot for the help. |
arpitkuriyal
left a comment
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.
LGTM!
jdesrosiers
left a comment
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.
Looks good! Vitest actually has special syntax for parameterized tests, but I don't see any reason why it would be preferable to use that over what you did here.
Fix: Add support for
formatkeyword across all JSON Schema dialectsThis PR updates the formatter to handle the
formatkeyword for every JSON Schema dialect that Hyperjump supports.What was wrong
Recent updates in @hyperjump/json-schema changed the format keyword to have dialect-specific URIs (2020-12, 2019-09, draft-07, draft-06, draft-04).The formatter still relied on the old URI, which no longer exists.As a result, tests like format: email were failing.
What I changed
1. Added normalization support for all dialect format URIs
https://json-schema.org/keyword/draft-2020-12/formathttps://json-schema.org/keyword/draft-2019-09/formathttps://json-schema.org/keyword/draft-07/formathttps://json-schema.org/keyword/draft-06/formathttps://json-schema.org/keyword/draft-04/format2. Updated error handler
The handler now checks for all dialect-specific format URIs, instead of assuming a single URI.
3. Added tests for all dialects
Added format: email tests for:
All tests now pass.
This PR fixes #90
Result
Formatting errors for
format: emailnow work consistently across all supported dialects.@jdesrosiers Please let me know if you'd like any follow-ups or adjustments.