Convert "Invalid source root" errors to UserErrors #1282
Conversation
src/init.ts
Outdated
| // Handle the situation where init is called twice | ||
| // for the same database in the same job. | ||
| // Check if we need to re-throw the error as a UserError in order to avoid | ||
| // counting this error towards our internal error budget. | ||
| if ( |
There was a problem hiding this comment.
Time to extract these conditions into their own shouldReportAsUserError helper function?
aeisenberg
changed the base branch from
main
to
aeisenberg/cli-config-processing
aeisenberg
force-pushed
the
aeisenberg/user-error-source-root
branch
from
7c40d0f
to
7c11f80
Compare
|
Rebasing in order to pick up a fix in integration tests from #1281. |
src/init.ts
Outdated
| return new util.UserError( | ||
| `Is the "init" action called twice in the same job? ${e.message}` | ||
| ); | ||
| } else if ( |
There was a problem hiding this comment.
Don't need the else since you're returning early.
src/init.ts
Outdated
| e.message?.includes("Invalid source root") | ||
| ) { | ||
| return new util.UserError(e.message); | ||
| } else { |
There was a problem hiding this comment.
Similar, this can just be a final fall-through return statement.
aeisenberg
force-pushed
the
aeisenberg/user-error-source-root
branch
from
7c11f80
to
e62ee58
Compare
| * attributed to the user, otherwise the original error. | ||
| */ | ||
| function processError(e: any): Error { | ||
| if (!(e instanceof Error)) { |
There was a problem hiding this comment.
Just curious: when would e not be an instanceof Error if it was sent from the try/catch block? Or is this just good practice in any error handling function?
There was a problem hiding this comment.
In javascript, you can "throw" anything. So you are not guaranteed that a catch clause is actually catching an Error instance. In practice, you should only throw errors. Typescript is just being pedantic here, and so is this code.
aeisenberg
force-pushed
the
aeisenberg/user-error-source-root
branch
from
befd353
to
9e044c5
Compare
This is an internal change. No changelog note required.
Merge / deployment checklist