Repository navigation
feat(prettier-plugin-toml): load tombi through tombit - #535
Conversation
Replace `@tombi-toml/lib` and `smol-toml` with `tombit`: configuration discovery and merging stay in Rust, explicit Prettier options are passed as `overrideConfig`, and the plugin drops its JavaScript discovery/caching layer. Prettier's resolved options (including its defaults) are authoritative for the rules the plugin exposes; `tombi.toml` still supplies the remaining rules and its `[[overrides]]` are applied by Tombi last. Diagnostics carry a range again, so syntax errors keep their code frame.
🦋 Changeset detectedLatest commit: d413da0 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 |
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 7 files. (1 skipped: 1 … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description Check | |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely identifies the main change: loading Tombi through tombit in the Prettier TOML plugin. |
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 7 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics -2 complexity · 0 duplication
Metric Results Complexity -2 Duplication 0
🟢 Coverage 100.00% diff coverage · -0.78% coverage variation
Metric Results Coverage variation ✅ -0.78% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (1dda891) 342 318 92.98% Head commit (e679712) 218 (-124) 201 (-117) 92.20% (-0.78%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#535) 8 8 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Deploying prettier with
|
| Latest commit: |
d413da0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://20a8312d.prettier-ans.pages.dev |
| Branch Preview URL: | https://feat-tombit-0-1-1.prettier-ans.pages.dev |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
size-limit report 📦
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #535 +/- ##
==========================================
- Coverage 92.25% 89.70% -2.55%
==========================================
Files 12 11 -1
Lines 413 272 -141
Branches 108 77 -31
==========================================
- Hits 381 244 -137
+ Misses 31 27 -4
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
prettier-plugin-autocorrect
prettier-plugin-pkg
prettier-plugin-sh
prettier-plugin-sql
prettier-plugin-toml
commit: |
Restore 100% coverage after dropping the JavaScript discovery layer: cover `useTabs`, explicit `stringQuoteStyle`/`inlineTableBraceSpaceWidth`, an unset `printWidth`, the missing-diagnostic guard and `locStart`/`locEnd`.
`tomlVersion` defaulted to `v1.0.0`, so the plugin always sent `toml-version` through `overrideConfig`; a project's `toml-version = "v1.1.0"` was then parsed as v1.0.0. Drop the option default and only emit the key when it is set. Refresh the docs/comments that still described the explicit-only precedence and drop the unused "explicitly configured" tuple flag.
6b813f1 to
88a43dd
Compare
Prettier can format a string without a filepath; fall back to a synthetic name so tombi can still discover a configuration instead of failing to convert `undefined` into a Rust string. Also simplify the core-rule mapping.
With no defaults, an unset plugin option is `undefined` and is not sent, so the discovered `tombi.toml` (or Tombi's own default) supplies it; explicitly set options still override. Prettier's core options keep their resolved values. The outputs without a config are unchanged (Tombi's defaults match the removed ones); added a test that the option definitions declare no defaults.
| async function format(code: string, options: PrettierOptions) { | ||
| const { formatted, diagnostics } = await formatToml(code, options.filepath, { | ||
| // Prettier may omit the filepath; a synthetic name still allows discovery. | ||
| const filepath = options.filepath || 'stdin.toml' |
There was a problem hiding this comment.
this can just be inlined.
There was a problem hiding this comment.
Inlined in 33e2f02: the filepath fallback is now an argument of the formatToml call.
Inline the filepath fallback and the four resolve helpers into `getCoreRules` as suggested in review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments: 11
packages/toml/src/config.ts: 2
35-37: LGTM!
15-22: 🎯 Functional Correctness
[email protected]derives an unsetkey-quote-stylefrom the effectivestring-quote-style. When the override changesstring-quote-style, the key quote style follows the override. No change is required.packages/toml/README.md: 1
53-93: LGTM!packages/toml/src/options.ts: 1
15-16: LGTM!packages/toml/test/config.spec.ts: 1
24-101: LGTM!packages/toml/test/options.spec.ts: 1
53-59: LGTM!packages/toml/package.json: 1
44-44: LGTM!packages/toml/src/index.ts: 1
42-48: LGTM!packages/toml/test/fixtures.spec.ts: 1
10-14: LGTM!packages/toml/test/index.spec.ts: 1
1-36: LGTM!.changeset/tombit.md: 1
19-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Fix the line wrap in the changeset text.
Line 19 is longer than the surrounding wrapped lines. It joins two sentences on one line. This does not change the rendered output. Rewrap it for consistency.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: defaults
- Review profile: CHILL
- Plan: Advanced
- Run ID:
c61ec8ec-80b3-497b-b088-94369715631a
⛔ Files ignored due to path filters: 1
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing: 13
.changeset/tombit.mdpackages/toml/README.mdpackages/toml/package.jsonpackages/toml/src/config.tspackages/toml/src/discover.tspackages/toml/src/index.tspackages/toml/src/options.tspackages/toml/test/config.spec.tspackages/toml/test/discover-global.spec.tspackages/toml/test/discover.spec.tspackages/toml/test/fixtures.spec.tspackages/toml/test/index.spec.tspackages/toml/test/options.spec.ts
💤 Files with no reviewable changes: 3
- packages/toml/test/discover.spec.ts
- packages/toml/test/discover-global.spec.ts
- packages/toml/src/discover.ts
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
i1stG
left a comment
i1stG
left a comment
There was a problem hiding this comment.
Review
The direction is good: dropping smol-toml and the 218-line JS discovery layer in favour of serde_tombi is a real simplification, and async format is a nice improvement. Tests, tsc and eslint are green.
Requesting changes for one blocking regression, plus a CI failure and some docs/semver issues.
🚨 Blocker: a discovered tombi.toml with [[overrides]] aborts the process
[email protected] panics natively and aborts:
thread '<unnamed>' panicked at tombi/rust/serde_tombi/src/ser.rs:155:17:
internal error: entered unreachable code: Document must be valid TOML.
thread caused non-unwinding panic. aborting.
With tombi.toml:
[format.rules]
indent-table-key-value-pairs = true
indent-width = 4
[[overrides]]
files.include = ["**/*.toml"]
[overrides.format.rules]
indent-width = 6and:
import { format } from 'prettier'
import plugin from 'prettier-plugin-toml'
await format('[t]\nkey = 1\n', {
filepath: '<dir>/a.toml',
parser: 'toml',
plugins: [plugin],
})Narrowed on tombit directly (Node 24.22.0, darwin/arm64):
| call | result |
|---|---|
config: false |
ok |
explicit config: { content, path } with [[overrides]] |
ok |
discovery with [[overrides]] (no overrideConfig) |
abort |
overrideConfig.overrides / explicit config + any overrideConfig |
abort |
4.0.0 (master) formats the same config fine (@tombi-toml/[email protected] + the previous JS merge), so this is a regression from this switch.
I filed the upstream repro at unrs/tombit#5. Please:
- add an
[[overrides]]regression test here, and - don't merge until
tombitis fixed (the^0.1.1range will pick up a 0.1.x patch).
CI
codecov/project fails: 92.25% → 89.70% (−2.55%).
Docs / semver
- The README precedence list says "each step overriding the previous one" but reads as discovered rules > Prettier options; the actual order is discovered base <
overrideConfig< per-file[[overrides]]. Reorder it or say "highest priority first". - The changeset is
minor, but this changes output for existing users (Prettier's defaults now beat a discoveredtombi.toml— e.g.line-width = 120stops applying unlessprintWidthmatches). That looksmajor.
Nits
index.spec.tsmockstombitbut never asserts the call args; worth locking in the'stdin.toml'fallback andoverrideConfig.fixtures.spec.ts's no-filepath test depends on cwd not containing atombi.toml.printWidth: Infinityomitsline-width, so a discoveredline-widthapplies despite "core options always win".
… review nits Reorder the precedence list in ascending priority, assert the synthetic filepath and the override config in the unit test, and run the no-filepath fixture test from a temporary cwd.
|
Thanks for the review.
|
Code Review ✅ Approved 2 closed / 2 findings🟡 Medium risk · Delegating TOML config resolution to tombit and changing option precedence could alter formatting or parsing unexpectedly for existing projects. Replaces ✅ 2 closed✅ Bug: Prettier's tomlVersion default now overrides tombi.toml's toml-version
✅ Quality: Docs/comments still say only explicit Prettier options are merged
VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Approved For more verification details, comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
tombit 0.1.3 applies a matching `[[overrides]]`'s format/lint rules over the base rules. Bump the dependency, add a plugin-level regression test, and note the requirement in the changeset.
|
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 2 complexity · 0 duplication
Metric Results Complexity 2 Duplication 0
🟢 Coverage 100.00% diff coverage · -0.78% coverage variation
Metric Results Coverage variation ✅ -0.78% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (1dda891) 342 318 92.98% Head commit (d413da0) 218 (-124) 201 (-117) 92.20% (-0.78%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#535) 8 8 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
i1stG
left a comment
i1stG
left a comment
There was a problem hiding this comment.
Re-review
The blocker is fixed and the review nits are addressed.
[email protected]now applies the discovered[[overrides]]: verified end-to-end (baseindent-width = 4, override6for both*.tomland**/*.toml→"[t]\n key = 1\n"), with no abort (async and sync).npx vitest run packages/toml→ 20/20, including the newoverrides.spec.ts;build:ts,lint:tscand eslint clean, CI matrix green.- README precedence list now reads correctly (base →
overrideConfig→ per-file[[overrides]]). - Nits covered: the no-filepath test chdirs to a temp dir, and
index.spec.tsasserts the syntheticstdin.toml+overrideConfig. - The precedence behavior change and the
minorchangeset decision are now explicitly documented in the PR body — owner's call, fine.
Non-blocking: codecov/project is still red (92.25% → 89.70%, from deleting the well-covered discovery layer); it isn't a required check, so not blocking.
LGTM.
What
Replaces
@tombi-toml/lib+smol-tomlwithtombit^0.1.3:.tombi.toml,tombi.toml,.config/tombi.toml,[tool.tombi]inpyproject.toml, then the user/system configs;discover.ts(218 lines) and thesmol-tomldependency;tombit'sformat/lintare async, so formatting no longer blocks the event loop;filepathnow falls back to a synthetic name for discovery instead of failing to convertundefinedinto a Rust string.Precedence
The rules are resolved in ascending priority: the discovered configuration's base rules, then Prettier's core options (
printWidth,tabWidth,useTabs,singleQuote,bracketSpacing) and explicitly set plugin options (passed asoverrideConfig), then the discovered configuration's per-file[[overrides]], which Tombi applies last.The plugin's own options no longer declare defaults, so "not set" is observable: an unset plugin option follows the discovered configuration (or Tombi's default). Prettier's core options are always resolved, so they cannot be distinguished from their defaults and stay authoritative.
toml-versionfollows the discovered configuration unless thetomlVersionoption is set, because it decides how the source is parsed.This is a behavior change from 4.0.0, where a discovered
tombi.tomlcould override Prettier's defaults.To keep the tombi CLI / VS Code extension in sync, set
line-widthintombi.tomlto the same value asprintWidth; with no configuration at all, Prettier defaults to80while tombi has no line width limit.Diagnostics
tombitexposesrange/sourceFileon diagnostics again, so syntax errors keep rendering their Prettier code frame.Review status
[[overrides]]abort (serialize the resolved config with the linkedtoml_edit) and applies a matching override's format/lint rules over the base rules (tombi-toml/tombi#2330).^0.1.3and adds a plugin-level[[overrides]]regression test (packages/toml/test/overrides.spec.ts, six-space indentation).codecov/projectreports about −2.4%: deleting a 97%-covered file lowers the project percentage while the remaining files' coverage andcodecov/patchare unchanged. No repo-wide Codecov config is added without the owner's decision.minor(the owner chose to avoid another major); the precedence change is documented above.Verification
yarn vitest run packages/toml— 20/20,toml/src100% coverageyarn lint:tsc,yarn eslint packages/toml— cleanfixture1.toml(80-column wrapping) andinvalid.toml(code frame) snapshots unchanged.changeset/tombit.md— minor