-
Notifications
You must be signed in to change notification settings - Fork 6
[DIT-10128] Better error messaging for config errors #120
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
39a9946 to
a2676c6
Compare
lib/src/index.ts
Outdated
| if (invalidKeys.length) { | ||
| errorText = `${ | ||
| error.data.messagePrefix | ||
| } Please remove or rename the following fields: ${invalidKeys.join( |
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.
I think instead of Please remove or rename the following fields:
it might make sense to simply identify the incorrect fields:
The following fields have issues: ...field names
What do you think?
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.
We could simplify to always just say that. But in this case, we know for certain the problem with those fields is that they exist at all (vs the other case where the fields are valid but their values are not). I was trying to be as helpful as possible without going too crazy deep into zod errors.
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.
ah gotcha that makes sense. In that case we can keep this like this
jaerod95
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.
I noticed it's only tracking the root level fields as invalid, i'm checking to see if there is a quick fix. Still testing but wanted to surface this
lib/src/index.ts
Outdated
| const invalidKeys = Array.from( | ||
| new Set( | ||
| error.data.issues | ||
| .map((issue) => ("keys" in issue ? issue.keys : null)) | ||
| .flat() | ||
| .filter(Boolean) | ||
| ) | ||
| ); |
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.
| const invalidKeys = Array.from( | |
| new Set( | |
| error.data.issues | |
| .map((issue) => ("keys" in issue ? issue.keys : null)) | |
| .flat() | |
| .filter(Boolean) | |
| ) | |
| ); | |
| const invalidKeys = Array.from( | |
| new Set( | |
| error.data.issues | |
| .map((issue) => | |
| "keys" in issue | |
| ? `${issue.path.join(".")}${ | |
| issue.path.length > 0 ? "." : "" | |
| }${issue.keys}` | |
| : null | |
| ) | |
| .flat() | |
| .filter(Boolean) | |
| ) | |
| ); |
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.
doing this will help identify if the issue is from a nested field or a top level field.
jaerod95
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.
Not shown in these changes, but to get better error messages for excess fields we'll need to add strict to the zod types in the src/outputs/json.ts file
jaerod95
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.
Great work!
Overview
Creates a new custom error class
DittoErrorthat allows use to pass specific error data from the call site to the error handler. This is similar toApiErrorin the main app, but with stronger typing since we're starting from scratch.Updates the error messages we surface to the user when we encounter a failure in the config file.
extra-- this seemed to be the best place to put unconstrained metadata, but I'm not sure how to test this.Context
https://linear.app/dittowords/issue/DIT-10128/improve-the-error-messaging-for-when-a-project-config-is-invalid
Test Plan
Testing successfully completed in via:
DEBUGto false so you only see what the user will seeconfig.ymlso it has a formatting/yaml error - you should see the right error messagenew DittoErrorcalls inprojectConfig.tsto include{ exitCode: 1 }, then trigger that error type - You should seeCommand failed with exit code 1instead of the default which is 2.