Final changes for parameter validation #7291
Conversation
| } | ||
| } | ||
|
|
||
| return message; |
There was a problem hiding this comment.
Do you know if we have enough info here to be able to log a link to the docs for the method the user tried to call? If the error is something like "Add more arguments", a link to the reference to see what's expected might be helpful. (Also not a blocker, but if it's possible to do, we can always make an issue for it and see if a community contributor is interested in taking it on)
There was a problem hiding this comment.
That's a great point, thank you for the suggestion! I just added a commit that would append a documentation link to the end of the error message for cases where the user provided too few / too many arguments. I think ideally I'd want to also make sure that the link is both valid and reachable before appending, but feel like that could be a TODO later since p5 documentation follows a pretty standard format, and I don't know how adding an async function in the code would affect it. For now, I think this would suffice.
…ided too few/too many arguments
| message = `Expected at least ${minArgs} argument${minArgs > 1 ? 's' : ''}, but received fewer. Please add more arguments!`; | ||
| break; | ||
| } | ||
| case 'invalid_type': { | ||
| message = buildTypeMismatchMessage(currentError.received, currentError.expected, currentError.path.join('.')); | ||
| break; | ||
| } | ||
| case 'too_big': { | ||
| const maxArgs = currentError.maximum; | ||
| message = `Expected at most ${maxArgs} argument${maxArgs > 1 ? 's' : ''}, but received more. Please delete some arguments!`; |
There was a problem hiding this comment.
The messages probably should have the name of the function being called in there to help with identifying source of error, if it is not already part of the message.
The suggested fix likely need to be more of a suggestive tone as the suggested fix may not be the actual problem. ie. instead of "Please add/delete arguments" go with something more like "Did you forget to add an argument?"/"Removing some arguments may help", or the suggestion can be omitted as well in line with the current FES's behavior.
|
For using the parameter validation throughout the code base, I mentioned not using |
|
Also for reference, here was an issue where we talk in the comments about a decorator approach with some code examples: #6597 |
…arameters, use decorator pattern
|
I think there's still some adjustment I would like to make to the messages but we can do that after phase two also has its messages. |
Addresses #7178
Changes:
paletteLerp, see added paletteLerp #6960zod-validation-errorlibrary, prints error messages with custom logic catered towards beginners, and modified the tests accordingly.convert.js, to 1) ignore false duplicated definitions (i.e. what happened withcreateAudio), and 2) dedup when overloads of the same types existprintZodSchemathat's only for debugging purposes, etc.Screenshots of the change:
PR Checklist
npm run lintpasses