-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add isoDate schema to zod and ajv
#63
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
base: main
Are you sure you want to change the base?
Conversation
kirillgroshkov
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.
Let's discuss in a call. The magic levels of this PR are nearing the maximum Hogwards-approved levels
|
|
||
| let error = 'Should be be a YYYY-MM-DD string' | ||
| if (after) error = `Should be after ${after}` | ||
| if (sameOrAfter) error = `Should be on or after ${sameOrAfter}` |
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.
p3: I prefer else if, which skips evaluating more conditions as soon as it encountered one truthy condition (since they are mutually exclusive)
|
|
||
| let schema = z.string().refine( | ||
| dateString => { | ||
| if (!localDate.isValidString(dateString)) return false |
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.
Just a note that I'm afraid of the performance implications.
In reality we deal with hundreds/thousands of isoDates per "validation run", so, it's very performance-sensitive code, I'd like to scrutinize it.
For example, we could get away with just string comparisons and not relying on LocalDate api.
isValidString can be instead done via regex?
Also, I'd like a fast-pass for case when there are no parameters
aa6b664 to
58e9f1d
Compare
1299b0b to
06656e9
Compare
Before anything: Ajv is one of the most badly documented tool relative to how much documentation is actually produced. It is badly worded, confusing.
In this PR, ChatGPT and I - in close collaboration - implement a parametrizable
isoDateschema for Zod and Ajv.It survives the Zod->Ajv transformation by carrying over the required parameters in the
descriptionfield of the JSON Schema.I suggest moving basic Ajv-related logic to
js-lib, since with the current setupjs-libimports intonodejs-libwhich is not a good direction - even though this is only in tests.This is because zod and ajv are very closely coupled for us, i.e. we should not implement a zod feature that will be missing in Ajv - so I have to keep these tests closely together.
By moving
getAjv()et al tojs-lib, this circular dependency would be solved.We can leave
AjvSchemainnodejs-lib, if we want