Catch errors from git being absent and continue on as best possible #173
Conversation
🦋 Changeset is good to goLatest commit: 74f553d We got this. Not sure what this means? Click here to learn what changesets are. |
ajaymathur
changed the title
ajaymathur
force-pushed
the
catch-git-errors
branch
from
acafe75
to
0e69cdb
Compare
packages/git/src/index.ts
Outdated
| try { | ||
| const masterRef = await getMasterRef(cwd); | ||
| return getChangedPackagesSinceCommit(masterRef, cwd); | ||
| } catch { | ||
| // Do nothing when operation fails | ||
| } | ||
| return []; |
There was a problem hiding this comment.
I was not able to get it tests in unit test properly. If you have encountered this type of condition please let me know the best way to test it.
There was a problem hiding this comment.
Since it should theoretically never happen, let's not wrap it in a try catch.
There was a problem hiding this comment.
If we don't wrap this in try catch, then error will bubble up and it will return an empty array they way we want it to.
There was a problem hiding this comment.
Oh, sorry, I thought this function was calling out to the other one that includes a try catch, makes sense.
There was a problem hiding this comment.
wrt testing: using cwd = await copyFixtureIntoTempDir(__dirname, "with-git") in a test but without running git init should make it fail.
There was a problem hiding this comment.
Cool, I will add a test.
packages/git/src/index.ts
Outdated
| try { | ||
| const masterRef = await getMasterRef(cwd); | ||
| return getChangedPackagesSinceCommit(masterRef, cwd); | ||
| } catch { | ||
| // Do nothing when operation fails | ||
| } | ||
| return []; |
There was a problem hiding this comment.
Since it should theoretically never happen, let's not wrap it in a try catch.
packages/git/src/index.ts
Outdated
| .filter(file => tester.test(file)); | ||
| if (!fullPath) return files; | ||
| return files.map(file => path.resolve(cwd, file)); | ||
| } catch {} |
There was a problem hiding this comment.
Could we make this specific to the git errors so we don't silently catch errors from actual potential bugs.
There was a problem hiding this comment.
Good pattern is:
try {
// some code at some time
} catch (e) {
if (e.message.includes('this-specific-thing') {
// handle the error
} else {
throw e
}
}message is one option, but there may be better things to interrogate.
There was a problem hiding this comment.
One thing I like doing is this
class GitError extends Error {}
try {
// some code at some time
throw new GitError('the message')
} catch (e) {
if (e instanceof GitError) {
// handle the error
} else {
throw e
}
}
ajaymathur
force-pushed
the
catch-git-errors
branch
from
0e69cdb
to
c86127d
Compare
ajaymathur
force-pushed
the
catch-git-errors
branch
from
c86127d
to
8dd0cf3
Compare
|
@Noviny @mitchellhamilton Thanks for the review, PR is ready for review again 🙂 |
|
@mitchellhamilton Thanks for adding changeset and updating readme. I actually added the package as |
Totally agree with having @changesets/errors(including the name), that update to the readme was making it consistent with the name in the package.json |
|
I read your commit in a wrong way. Thanks for correcting it, 🙂 |
No description provided.