Update to param validator + test file #7194
Conversation
| // ... | ||
| // } | ||
| const constantsMap = {}; | ||
| for (const [key, value] of Object.entries(constants)) { |
There was a problem hiding this comment.
Just a thought for our module restructuring effort, @limzykenneth when we're thinking of moving constants into their relevant modules instead of one big file, will we have to do anything special to continue supporting getting a big list of all of them?
There was a problem hiding this comment.
Hmm...that's a good question. It might be that module need to specifically register things as constants but I want to avoid that where possible to simplify things.
There was a problem hiding this comment.
Is it possible to solely rely on the documentation parsed data to get this list? We can possibly add extra tags or into into the JSDoc comments if needed to mark something as a constant.
There was a problem hiding this comment.
It might not be possibly to get it from the parameter data because the types only include the names of the constants, and we won't know from that what value they correspond to (like what specific symbol to check against)
There was a problem hiding this comment.
If we switch back to using strings, I think it could be doable though?
There was a problem hiding this comment.
We can possibly switch back to strings, the symbol stuff I'm not 100% sold on yet as I'm also not sure how or if it should work across multiple instances.
| return z.union(types | ||
| .filter(t => { | ||
| if (!schemaMap[t]) { | ||
| console.log(`Warning: Zod schema not found for type '${t}'. Skip mapping`); |
There was a problem hiding this comment.
Do you think we should make this throw an error so that it's obvious when we add a method that doesn't have a Zod schema yet?
There was a problem hiding this comment.
Yes absolutely! I used console.log right now because many objects are not supported yet. Added a TODO here for future reference!
| // parameters that allow multiple types. | ||
| schema = types.every(t => /^[A-Z_]+$/.test(t)) | ||
| ? generateConstantSchema(types) | ||
| : generateUnionSchema(types); |
There was a problem hiding this comment.
Does this case also handle constants if they exist? e.g. something like Number|AUTO maybe? If not, maybe we can use generateUnionSchema for everything and update its map to map to a z.literal if a type is a constant.
There was a problem hiding this comment.
That's a great point! I just added another commit that changes the logic of generating schema for each parameter and moved things around accordingly.
Addresses #7178
Changes:
Screenshots of the change:
Ran
npm run test:devlocally and all the tests now passNote:
I have yet to implement parameter validation that requires a browser environment (i.e. for web API objects and p5 objects), that will be done in a follow-up PR.
PR Checklist
npm run lintpasses