Unify minimum/maximum error handlers with draft-04 compatibility#163
Unify minimum/maximum error handlers with draft-04 compatibility#163jdesrosiers merged 4 commits intohyperjump-io:mainfrom
Conversation
jdesrosiers
left a comment
jdesrosiers
left a comment
There was a problem hiding this comment.
The new error handlers look like the right idea, but there's a lot of cleanup that needs to be done everywhere else.
- You need to rebase your branch and resolve conflicts
- The old handlers need to be removed
- There's no need for new normalization handlers
- There should be no changes to
hyperjump-json-schema.test.js
|
Thank you for the feedback. Regarding the changes to |
Padmashree06
force-pushed
the
exclusive-min-max
branch
from
89f1f76 to
2d7cd58
Compare
|
I have removed the individual handlers and their corresponding normalization handlers, reverted the changes made in Please review |
jdesrosiers
left a comment
jdesrosiers
left a comment
There was a problem hiding this comment.
You still haven't removed the maximum and minimum handlers. And you're still using a combined normalization handler. Normalization handlers are never combined, only error handlers. That's why they are defined separately.
…rmalization handlers
Padmashree06
force-pushed
the
exclusive-min-max
branch
from
2d7cd58 to
1ab56dd
Compare
|
Thank you for the feedback. |
jdesrosiers
left a comment
jdesrosiers
left a comment
There was a problem hiding this comment.
The new tests look great, but the one you changed isn't correct. Everything else looks good. I also asked for a few code style updates to better match the rest of the code base.
| const combinedMaximumErrorHandler = async ( | ||
| normalizedErrors, | ||
| instance, | ||
| localization | ||
| ) => { |
There was a problem hiding this comment.
Please collapse this to one line.
| /** @type (pointer: string) => string */ | ||
| const pointerPop = (pointer) => pointer.replace(/\/[^/]+$/, ""); | ||
|
|
||
| export default combinedMaximumErrorHandler; |
There was a problem hiding this comment.
I don't think we need to call this "combined". It can just be "maximumErrorHandler". (Same for the minimum handler.)
src/localization.js
Outdated
| /** @type (exclusiveMaximum: number) => string */ | ||
| getExclusiveMaximumErrorMessage(exclusiveMaximum) { | ||
| return this.#formatMessage("exclusiveMaximum-message", { exclusiveMaximum }); | ||
| return this.#formatMessage("exclusiveMaximum-message", { exclusiveMaximum, maximum: exclusiveMaximum }); |
There was a problem hiding this comment.
This change doesn't appear to be necessary. (Same for minimum.)
src/test-suite/tests/maximum.json
Outdated
| "schemaLocations": [ | ||
| "#/maximum", | ||
| "#/exclusiveMaximum" | ||
| "#/maximum" |
There was a problem hiding this comment.
This isn't right. In this case, both keywords need to be in schemaLocations because exclusiveMaximum alters maximum. After draft-04, it can be just the one that's most specific.
|
Thank you for the review.
Please let me know if there are any other changes to be made! |
|
Thank you for merging! |
2 participants
Description
This PR introduces unified error handlers for:
minimum,exclusiveMinimumanddraft-04 minimummaximum,exclusiveMaximumanddraft-04 maximumThe handlers now select and report only the most restrictive constraint, producing a single validation error instead of multiple overlapping messages.
Additional Updates
index.jsto avoid overlap and can be removed if preferred.Tests
All tests pass locally.
Fixes
Issue: #135