Repository navigation
Ignore find changed packages error in changeset add - #1485
Conversation
🦋 Changeset detectedLatest commit: d95273c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1485 +/- ##
==========================================
- Coverage 81.07% 81.01% -0.06%
==========================================
Files 54 54
Lines 2224 2228 +4
Branches 662 662
==========================================
+ Hits 1803 1805 +2
- Misses 417 419 +2
Partials 4 4 ☔ View full report in Codecov by Sentry. |
| cwd, | ||
| }) | ||
| ).map((pkg) => pkg.packageJson.name); | ||
| } catch (e: any) { |
There was a problem hiding this comment.
I think we should strive to handle specific errors here. We can throw custom errors in @changesets/git and handle them here
There was a problem hiding this comment.
Do you mean we should only handle the specific error from getDivergedCommit, and otherwise we re-throw the other errors?
There was a problem hiding this comment.
I looked through the call paths for getVersionableChangedPackages and it looks like there's many spawns or git operations that could throw a generic error that fails, so we'd have to go through more of the codebase to update the error class.
I think my intention with the catch-all here is also because the changedPackagesNames is a best-effort call to show users changed packages for easier selection. If any error happens there it shouldn't block creating changesets at all. So I think a catch-all may still be feasible here?
There was a problem hiding this comment.
Hm, I see your point. It feels like it would be good to at least avoid displaying the error stack for those errors that we know of.
I'm fine with this change as-is though - it's up to you if you think it's worth changing anything here. I'd add a code comment about the catch-all rationale here though
There was a problem hiding this comment.
It feels like it would be good to at least avoid displaying the error stack for those errors that we know of.
That sounds like a good idea to me. I think I'll follow-up using custom errors to achieve that and still leave the catch-all logging for errors we don't expect.
Will add a comment for the catch-all 👍
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Resolves the issue noted in #1473 (comment). If
baseBranchcan't be found locally, you'd no longer get a hard error, but a warning.Before:
After:
