Make formatter a required option for config bucket#4
Conversation
…r non js/ts config files
github-actions
bot
added
dependencies
🦋 Changeset detectedLatest commit: 615afcf The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughFormatters are now required in the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/config/src/shapes.ts (1)
25-36:⚠️ Potential issue | 🟠 MajorReconsider version bump strategy for this breaking change.
The changeset specifies
patch, but makingformatterrequired is a breaking change. Previously,formatterwas optional with automatic fallback to@saykit/format-po; users who didn't explicitly configure a formatter now encounter validation errors. For a 0.0.0 package, this warrants aminorversion bump under pre-release semantic versioning conventions (ormajorfor stricter interpretation). Whilst patch versions are technically permitted pre-1.0, they conventionally signal backward compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/shapes.ts` around lines 25 - 36, The schema change to make formatter required in the Bucket zod schema (symbol: Bucket, field: formatter, type: Formatter) is a breaking change for consumers who relied on the previous optional fallback to `@saykit/format-po`; either revert the schema to keep formatter optional (restore z.optional() or provide a default/fallback in the transform) or update the release metadata to a non-patch bump (e.g., bump to minor/major in the changeset) to reflect the breaking change; locate the Bucket definition and adjust the formatter property or the changeset accordingly.
🧹 Nitpick comments (1)
packages/config/src/features/loader/resolve.ts (1)
11-12: Improve unsupported-type error clarity for extension-less filesNice fail-fast check. Small tweak: at Line 12, when
ext === '', the message renders as"", which is a bit cryptic. Consider mapping that case to"<no extension>"and including supported extensions to make fixes quicker.💡 Suggested patch
const ext = extname(file.id).toLowerCase(); const loader = ext in loaders ? loaders[ext as keyof typeof loaders] : null; - if (!loader) throw new Error(`Unsupported config file type "${ext}" for "${name}"`); + if (!loader) { + const fileType = ext || '<no extension>'; + const supported = Object.keys(loaders).join(', '); + throw new Error( + `Unsupported config file type "${fileType}" for "${name}". Supported types: ${supported}` + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/config/src/features/loader/resolve.ts` around lines 11 - 12, The error thrown when no loader is found (the block using loaders, loader, ext and name) should map an empty ext to a friendly "<no extension>" label and append the list of supported extensions to the message; update the thrown Error in the check that uses ext in loaders ? loaders[ext as keyof typeof loaders] : null to build a displayExt variable like ext || "<no extension>" and include Object.keys(loaders) (or a joined string of supported extensions) in the Error text so it reads something like Unsupported config file type "<displayExt>" for "<name>" — supported: [..].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/config/src/shapes.ts`:
- Around line 25-36: The schema change to make formatter required in the Bucket
zod schema (symbol: Bucket, field: formatter, type: Formatter) is a breaking
change for consumers who relied on the previous optional fallback to
`@saykit/format-po`; either revert the schema to keep formatter optional (restore
z.optional() or provide a default/fallback in the transform) or update the
release metadata to a non-patch bump (e.g., bump to minor/major in the
changeset) to reflect the breaking change; locate the Bucket definition and
adjust the formatter property or the changeset accordingly.
---
Nitpick comments:
In `@packages/config/src/features/loader/resolve.ts`:
- Around line 11-12: The error thrown when no loader is found (the block using
loaders, loader, ext and name) should map an empty ext to a friendly "<no
extension>" label and append the list of supported extensions to the message;
update the thrown Error in the check that uses ext in loaders ? loaders[ext as
keyof typeof loaders] : null to build a displayExt variable like ext || "<no
extension>" and include Object.keys(loaders) (or a joined string of supported
extensions) in the Error text so it reads something like Unsupported config file
type "<displayExt>" for "<name>" — supported: [..].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0c87684-3dcb-4c92-95eb-4e2e10d9bf4e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.changeset/plain-oranges-wonder.mdexamples/carbon-tsdown/package.jsonexamples/carbon-tsdown/saykit.config.jsonexamples/carbon-tsdown/saykit.config.tsexamples/nextjs-babel/package.jsonexamples/nextjs-babel/saykit.config.tspackages/config/package.jsonpackages/config/src/features/compile.tspackages/config/src/features/extract.tspackages/config/src/features/loader/explorer.tspackages/config/src/features/loader/loaders.tspackages/config/src/features/loader/resolve.tspackages/config/src/shapes.tspackages/config/tsdown.config.ts
💤 Files with no reviewable changes (5)
- examples/carbon-tsdown/saykit.config.json
- packages/config/package.json
- packages/config/src/features/loader/explorer.ts
- packages/config/tsdown.config.ts
- packages/config/src/features/loader/loaders.ts
1 participant
formatterfield of bucket config requiredSummary by CodeRabbit
New Features
Chores