Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 1fdb366 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 |
✅ Deploy Preview for changesets ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2315 +/- ##
==========================================
+ Coverage 87.48% 87.55% +0.06%
==========================================
Files 78 78
Lines 2973 2988 +15
Branches 853 854 +1
==========================================
+ Hits 2601 2616 +15
Misses 337 337
Partials 35 35 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi! This has been open for three weeks without a review, so I wanted to check whether anything else is needed from my side. It closes #2101: a changeset that names only packages which won't be released (ignored, or private with CI is green and the branch is conflict-free — happy to reshape it if you'd prefer a different surface (a hard error, a flag, or a different message) for that case. |
beeequeue
left a comment
There was a problem hiding this comment.
the changes make it only warn when a changeset only includes skipped packages, not if it includes any skipped package. that is not what the original issue was about, nor your PR description and limits the warning massively so a large number of cases would be let through when the warning could've helped.
the code is clearly ai generated and you don't seem to have looked at it before opening the pr, but i'll just leave this review for now as we don't have an ai policy yet
| expect(process.exit).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("should warn when a changeset only references packages that are not going to be released", async () => { |
There was a problem hiding this comment.
change the test to whether it warns when "there are any skipped packages in a changeset" instead of "all packages are skipped"
There was a problem hiding this comment.
Done for the wording, but I couldn't write it as a mixed changeset: assembleReleasePlan rejects a changeset that mixes skipped and non-skipped packages (Found mixed changeset ..., see packages/assemble-release-plan/src/index.test.ts:404) before status gets a chance to warn, so that combination can't reach the warning. The test covers a changeset whose packages are all skipped, which is what the warning can actually see today.
| "@changesets/cli": patch | ||
| --- | ||
|
|
||
| Warn when `changeset status` finds changesets that don't list any package which is going to be released. |
There was a problem hiding this comment.
| Warn when `changeset status` finds changesets that don't list any package which is going to be released. | |
| Warn when `changeset status` finds changesets with a skipped package which will not be released. |
|
|
||
| log.warn( | ||
| ` | ||
| The following changesets don't list any package that's going to be released, so they have no effect: |
There was a problem hiding this comment.
| The following changesets don't list any package that's going to be released, so they have no effect: | |
| The following changesets contain a package that is not going to be released as they are skipped: |
| // A changeset that lists an ignored/private package is dropped from the plan | ||
| // without any trace. One that lists nothing but such packages therefore has | ||
| // no effect at all, which is worth pointing out - typos already error, | ||
| // but choosing a package that isn't going to be released doesn't. |
There was a problem hiding this comment.
not formatted in a logical way, and should've been placed at the function instead of a random line inside of it
There was a problem hiding this comment.
Reworked per the review:
- the condition is now "any package in the changeset is skipped" instead of "every package is skipped", and the warning lists just the skipped names from each changeset rather than every package in it
- moved the explanatory comment above the function (it was sitting in the middle of it) and used your wording for the message
- changeset entry updated to your suggestion
One thing I ran into while changing the condition, so it isn't a surprise in the test: a changeset that names both a skipped and a non-skipped package never reaches this warning, because assembleReleasePlan throws Found mixed changeset ... first (packages/assemble-release-plan/src/index.test.ts:404 covers it). So today the warning fires for changesets whose packages are all skipped — which is the case the issue was filed about — and the test keeps covering that. If you'd rather changeset status listed the ignored entries of a mixed changeset instead of the plan rejecting it, that's a change in assemble-release-plan rather than here, so I left it alone; happy to do it if you want it.
Fair hit on the comment placement as well, that was scaffolding I left in before opening.
|
All three inline notes are in the 2026-10-12 push: the filter is now One yes/no worth having when you get a chance: whether No rush on the re-review. |
A changeset that lists a package which isn't going to be released (
ignore, or a private package withprivatePackages.versionturned off) is dropped from the release plan without any trace, so a changeset that names nothing but such packages silently has no effect. An unknown package name already fails hard, so this adds the same kind of feedback for that case:changeset statusnow warns and lists the changesets in question.Nothing fails and the
--outputresult is unchanged, so existing CI usage is unaffected.Closes #2101