Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
🦋 Changeset detectedLatest commit: 26f5e45 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
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 |
beeequeue
force-pushed
the
valibot
branch
2 times, most recently
from
9f5718b to
be4d2b4
Compare
There was a problem hiding this comment.
i need to test this against some complex configs from users
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## next #2015 +/- ##
==========================================
- Coverage 81.27% 80.62% -0.66%
==========================================
Files 55 61 +6
Lines 2377 2317 -60
Branches 718 638 -80
==========================================
- Hits 1932 1868 -64
- Misses 394 397 +3
- Partials 51 52 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| if (errors.length !== 0) { | ||
| throw new ExitError(1, { cause: new Error(messages.join("\n")) }); |
There was a problem hiding this comment.
I thought this would print the errors twice but we'll ignore this cause here:
changesets/packages/cli/src/index.ts
Lines 45 to 48 in bd27256
Attaching the .cause is nice - but perhaps this could actually throw that removed ValidationError? It would give it more context.
And I'm nitpicky here... but this message should just use:
| throw new ExitError(1, { cause: new Error(messages.join("\n")) }); | |
| throw new ExitError(1, { cause: new Error(errors.join("\n")) }); |
- coloring shouldn't affect this
- warnings are not the cause of the error and they are part of the
messagesnow
There was a problem hiding this comment.
i would say that ValidationError could be useful IF we expected anyone to run the CLI programmatically, which we don't.
since the CLI can only exit and print errors we should use ExitError.
the .cause shows up in vitest (iirc) so it's helpful for bubbling up the error without surfacing it in the CLI.
👍
| const logFn = errors.length === 0 ? log.error : log.warn; | ||
| if (messages.length !== 0) { | ||
| logFn(`- ${messages.join("\n- ")}`); | ||
| } |
There was a problem hiding this comment.
I feel like both diagnostic "levels" (warn/error) should just use different log levels and not be put into the same category here
There was a problem hiding this comment.
this is a detail for how the output will look, we can polish it when we polish the rest of the CLI visuals
bluwy
left a comment
bluwy
left a comment
There was a problem hiding this comment.
Thanks for cleaning this up. I really appreciate it! This looks good to me now.
3 participants
this PR reworks the config package to use Valibot for config shape validation, and then a rules engine(-ish) for validating that the config is correct.
this PR does NOT remove the remaining
console.logs from various packages, as i wanted to keep this PR slightly more focused