fix: refactor embedded options and embedder logic #157
Conversation
🦋 Changeset detectedLatest commit: 49b5595 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 |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds three-mode newline handling and alignment helpers to embedding rehydration, updates embedder callsites and exports, reorders some imports/exports, upgrades tooling and CI, replaces the test fixture runner with a per-language harness, and adds many language-specific fixtures and options files. (30 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/embedded/json/options.ts (1)
2-10: Import reordering is purely cosmetic.The type import
StringListToInterfaceKeyhas been moved within the same import block. This change has no impact on runtime behavior or the public API.src/embedded/nginx/options.ts (1)
2-10: Import reordering is purely cosmetic.The type import
StringListToInterfaceKeyhas been repositioned within the same import block. This change has no functional impact.src/embedded/java/options.ts (1)
2-10: Import reordering is purely cosmetic.The type import
StringListToInterfaceKeyhas been repositioned within the same import block, with no functional impact.src/embedded/xml/options.ts (1)
2-10: Import reordering is purely cosmetic.The type import
StringListToInterfaceKeyhas been repositioned within the same import block, consistent with the pattern across other embedded options modules.src/embedded/sql/options.ts (1)
2-12: Import reordering is purely cosmetic.The type import
StringListToInterfaceKeyhas been repositioned within the same import block, completing the consistent import-order standardization across all embedded options modules.src/types.ts (1)
8-17: Import reordering is purely cosmetic.The
PluginEmbedOptionsimport has been repositioned within the same import block. This change has no functional impact and aligns with the broader import-order standardization across the PR.tests/fixtures.spec.ts (2)
22-28: Silent error swallowing may hide configuration issues.When
JSON.parsefails inloadLanguageOptions, the error is silently caught and an empty object is returned. Consider logging a warning similar toloadPluginsto help diagnose misconfiguredoptions.jsonfiles during test development.} catch { + console.warn(`Failed to parse options.json in ${languageDir}`); return {}; }
123-126: Unnecessary array spread.The
[...plugins]spread creates a copy that isn't needed sinceloadPluginsalready creates a new array internally. You can passpluginsdirectly.- const languagePlugins = await loadPlugins([...plugins]); + const languagePlugins = await loadPlugins(plugins);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltests/__snapshots__/fixtures.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (53)
.changeset/cuddly-webs-chew.md(1 hunks)CHANGELOG.md(0 hunks)biome.json(2 hunks)package.json(1 hunks)scripts/import-ts-module-worker.ts(1 hunks)src/embedded/css/embedder.ts(1 hunks)src/embedded/css/options.ts(1 hunks)src/embedded/es/options.ts(1 hunks)src/embedded/glsl/options.ts(1 hunks)src/embedded/graphql/options.ts(1 hunks)src/embedded/html/options.ts(1 hunks)src/embedded/index.ts(1 hunks)src/embedded/ini/options.ts(1 hunks)src/embedded/java/embedder.ts(1 hunks)src/embedded/java/options.ts(1 hunks)src/embedded/json/options.ts(1 hunks)src/embedded/jsonata/options.ts(1 hunks)src/embedded/latex/options.ts(1 hunks)src/embedded/markdown/embedder.ts(4 hunks)src/embedded/markdown/options.ts(1 hunks)src/embedded/nginx/options.ts(1 hunks)src/embedded/pegjs/options.ts(1 hunks)src/embedded/php/options.ts(1 hunks)src/embedded/prisma/options.ts(1 hunks)src/embedded/properties/options.ts(1 hunks)src/embedded/pug/embedder.ts(1 hunks)src/embedded/pug/options.ts(1 hunks)src/embedded/ruby/options.ts(1 hunks)src/embedded/sh/embedder.ts(1 hunks)src/embedded/sh/options.ts(1 hunks)src/embedded/sql/embedder.ts(1 hunks)src/embedded/sql/options.ts(1 hunks)src/embedded/toml/embedder.ts(1 hunks)src/embedded/toml/options.ts(1 hunks)src/embedded/ts/options.ts(1 hunks)src/embedded/utils.ts(4 hunks)src/embedded/xml/options.ts(1 hunks)src/embedded/yaml/options.ts(1 hunks)src/index.ts(1 hunks)src/printers.ts(1 hunks)src/types.ts(1 hunks)src/utils.ts(3 hunks)tests/fixtures.spec.ts(1 hunks)tests/fixtures/css/issue-114.js(1 hunks)tests/fixtures/css/issue-88.js(1 hunks)tests/fixtures/markdown/issue-120.js(1 hunks)tests/fixtures/sql/issue-103.js(1 hunks)tests/fixtures/sql/issue-137.js(1 hunks)tests/fixtures/sql/issue-35.js(1 hunks)tests/fixtures/sql/issue-45.js(1 hunks)tests/fixtures/sql/issue-62.ts(1 hunks)tests/fixtures/sql/issue-64.js(1 hunks)tests/fixtures/sql/options.json(1 hunks)
💤 Files with no reviewable changes (1)
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/utils.ts (2)
src/index.ts (1)
EmbeddedOverride(11-11)src/types.ts (1)
EmbeddedOverride(102-105)
src/embedded/utils.ts (1)
src/types.ts (1)
InternalPrintFun(20-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (54)
src/embedded/glsl/options.ts (1)
8-8: LGTM: Import reordering for consistency.The import reordering is purely cosmetic and aligns with the pattern applied across multiple embedded language option modules in this PR.
.changeset/cuddly-webs-chew.md (1)
1-5: LGTM: Changeset correctly documents the fixes.The changeset appropriately describes the indentation fixes for embedded languages and uses the correct patch bump level.
src/embedded/pug/options.ts (1)
8-8: LGTM: Import reordering for consistency.The import reordering matches the standardization pattern applied across embedded language option modules.
scripts/import-ts-module-worker.ts (1)
8-11: LGTM: Formatting improvement.The multiline formatting of the parameter type annotation improves readability without affecting runtime behavior.
tests/fixtures/sql/issue-137.js (1)
1-1: LGTM: Appropriate test directive.The Prettier directive correctly configures the embedded SQL tag for this test fixture.
src/embedded/yaml/options.ts (1)
8-8: LGTM: Import reordering for consistency.The import reordering maintains consistency with the standardization pattern across embedded language option modules.
tests/fixtures/css/issue-88.js (1)
1-6: LGTM: Test fixture for plugin enablement behavior.The fixture appropriately tests issue #88 by configuring an empty
embeddedCssTagsarray, which should prevent the plugin from treating thecsstag as embedded CSS content. This helps verify that plugin enablement doesn't unexpectedly alter formatting when tags are explicitly excluded.src/embedded/toml/embedder.ts (1)
52-57: Verify the newline handling mode change.The change from boolean
trueto string"hardline"represents a semantic shift in how newlines are handled during document rehydration. This aligns with a broader refactor introducing semantic modes ("hardline", "literalline") to replace boolean flags. The function signature validates this with parameter typefalse | "hardline" | "literalline", where "hardline" replaces newlines with hardline formatting (adds indentation) and "literalline" uses replaceEndOfLine (preserves original indentation). Usage is consistent across embedders: toml, markdown, sql, sh, pug, and java use "hardline"; css uses "literalline"; others default to false.src/embedded/jsonata/options.ts (1)
2-9: LGTM!The import reordering is purely stylistic with no behavioral impact.
src/printers.ts (1)
127-147: LGTM!The explicit initialization of
stringFormTagtoundefinedis clear and the subsequent logic correctly handles the matching flow for complex tag expressions.src/embedded/ts/options.ts (1)
2-10: LGTM!The import reordering is purely stylistic with no behavioral impact.
src/embedded/pegjs/options.ts (1)
2-9: LGTM!The import reordering is purely stylistic with no behavioral impact.
src/embedded/java/embedder.ts (1)
52-57: LGTM - consistent with broader refactor.The change from
trueto"hardline"aligns with the coordinated refactor across embedders. The mode appropriately preserves hard line breaks for Java code formatting.tests/fixtures/sql/issue-64.js (1)
1-6: Test fixture is correct and properly addresses the issue.The test fixture correctly demonstrates member expression tags (
db.sql) for SQL embedding with parameterized queries. Issue #64 requests support for object properties as identifiers (member expressions likeSyntax.js), which is exactly what this test case validates. The fixture is appropriately included in the PR scope.Likely an incorrect or invalid review comment.
tests/fixtures/sql/issue-45.js (1)
1-6: Test fixture correctly addresses multi-line dollar-quoted string formatting.The test for issue #45 is appropriately located and references a real issue in the prettier-plugin-embed repository concerning unstable formatting of multi-line dollar-quoted strings. The fixture correctly demonstrates the SQL formatting scenario that issue #45 addresses.
src/embedded/pug/embedder.ts (1)
52-57: The newline handling mode change for Pug is correct and appropriate.The parameter was refactored from
true(boolean) to"hardline"(semantic union type), which replaces newlines with hardline breaks and re-indentation. This is the correct mode for Pug, as the prettier plugin produces output from column 0 that requires proper re-indentation, consistent with other similar languages like SQL and Shell.tests/fixtures/sql/issue-62.ts (1)
1-8: LGTM! Well-structured test fixture.This test fixture appropriately exercises tagged templates with TypeScript type parameters, directly addressing issue #62.
tests/fixtures/sql/issue-35.js (1)
1-5: LGTM! PostgreSQL-specific syntax test.This fixture appropriately tests PostgreSQL's type casts, square-bracket identifiers, and dollar-quoted string literals, addressing issue #35.
biome.json (3)
4-4: LGTM! Improved file matching with fixture exclusion.The change from
files.ignoretofiles.includeswith a negated pattern explicitly excludes test fixtures from linting/formatting, which aligns well with the expanded test coverage in this PR.
7-7: LGTM! Updated to newer Biome API.The migration from the deprecated
organizeImportsconfiguration toassist.actions.source.organizeImportsaligns with Biome's current API surface.
33-33: LGTM! Recursive patterns improve coverage.The change from single-level patterns (
*.json,package.json) to recursive patterns (**/*.json,**/package.json) ensures consistent formatting across nested directories.Also applies to: 41-41
src/utils.ts (2)
192-192: LGTM! Explicit return type improves type safety.The added return type annotation makes the function signature clearer and helps catch type mismatches at compile time.
517-517: Object.hasOwn is supported by this project's target environment.The project's
tsconfig.base.jsonexplicitly sets"target": "ESNext", which includes ES2022 and natively supportsObject.hasOwnin Chrome 93+, Firefox 92+, Safari 15.4+ and Node.js 16.9.0+. No polyfill is needed.Likely an incorrect or invalid review comment.
src/embedded/html/options.ts (1)
9-9: Import reordering - stylistic change only.This change reorders the type import without affecting functionality.
src/embedded/sh/options.ts (1)
8-8: Import reordering - stylistic change only.This change reorders the type import without affecting functionality, consistent with similar changes across other options files.
src/embedded/toml/options.ts (1)
8-8: Import reordering - stylistic change only.This change reorders the type import without affecting functionality, consistent with similar changes across other options files.
src/embedded/sql/embedder.ts (1)
63-68: ThesimpleRehydrateDocsignature change is correct and consistent.The fourth parameter now accepts
false | "hardline" | "literalline"instead of a boolean. The change in sql/embedder.ts from booleantrueto string"hardline"is valid and matches the refactored API signature. This pattern is consistently used across other embedders (markdown, sh, css).src/embedded/es/options.ts (1)
2-10: Import reordering looks good.The repositioning of
type StringListToInterfaceKeywithin the import block is a cosmetic change that maintains consistency across embedded option modules. No functional impact.src/embedded/css/options.ts (1)
2-10: LGTM!Consistent import reordering matching the pattern across other embedded option modules.
src/embedded/php/options.ts (1)
2-9: LGTM!Import reordering is consistent with the PR-wide pattern.
src/embedded/ruby/options.ts (1)
2-10: LGTM!Consistent import reordering with no functional changes.
src/embedded/ini/options.ts (1)
2-9: LGTM!Import reordering aligns with the consistent pattern across embedded option modules.
src/embedded/latex/options.ts (1)
2-9: LGTM!Import reordering is consistent with other embedded option modules.
src/embedded/prisma/options.ts (1)
2-9: LGTM!Import reordering maintains consistency with the PR-wide pattern.
tests/fixtures/sql/options.json (1)
1-5: No issues found. The test fixture correctly usesprettier-plugin-sqlas the plugin name, and theembeddedOverridesvalue is properly formatted as a JSON-encoded string, which is the expected configuration format for prettier-plugin-embed.tests/fixtures/css/issue-114.js (1)
1-10: Well-structured test fixture for issue #114.This fixture appropriately tests the CSS comment formatting stability issue. The multi-line comment with a
grid-template-columnsrule provides a good test case to ensure repeated formatting runs don't cause indentation drift.src/embedded/sh/embedder.ts (1)
52-57: The third argument accepts string values, not booleans—ensure correct parameter usage.The
simpleRehydrateDocfunction acceptsnewlineHandling: false | "hardline" | "literalline" = false, not booleantrue. The current use of"hardline"here is correct, replacing newlines with hardline formatting to add proper indentation. This aligns with similar changes across the embedder modules (sql, pug, toml, markdown, java) for consistent handling of embedded content indentation.Likely an incorrect or invalid review comment.
src/embedded/markdown/options.ts (1)
2-10: Import reordering looks good.The
StringListToInterfaceKeytype import is reordered to maintain consistent import ordering across embedded option modules. No functional changes.src/embedded/properties/options.ts (1)
2-9: Consistent import ordering.Same import reordering pattern applied here. No functional changes.
src/embedded/graphql/options.ts (1)
2-9: Consistent import ordering.Same import reordering pattern as other options files. No functional changes.
tests/fixtures/sql/issue-103.js (1)
1-19: Good test coverage for issue #103.The fixture correctly reproduces both scenarios from the issue:
- Interpolated expression in WHERE/IN clause
- Interpolated expression in FROM clause for dynamic table names
These cases exercise the multi-line nested
sql()indentation that was previously broken.tests/fixtures/markdown/issue-120.js (1)
1-17: Good test coverage for issue #120.The fixture correctly reproduces the nested block scenario where
noEmbeddedMultiLineIndentationshould prevent additional indentation from being added to embedded Markdown content. The@prettierdirective properly configures the required options.src/embedded/css/embedder.ts (1)
55-62: Good fix for CSS comment indentation stability.The switch from boolean
trueto"literalline"aligns with Prettier's internal CSS embedding behavior and addresses issue #114 (unstable formatting of CSS comments). ThesimpleRehydrateDocAPI accepts this value and explicitly documents that it "preserves original indentation" via replaceEndOfLine, which directly solves the progressive indentation issue.package.json (1)
83-131: LGTM on dependency updates.The dependency updates align with the broader tooling changes in this PR. The version ranges using
^allow minor/patch updates while pinning majors appropriately.src/embedded/markdown/embedder.ts (2)
88-94: Good fix for issue #120 - prevents unwanted indentation in markdown templates.The new early-return path using
literallineanddedentToRootcorrectly addresses the reported issue where markdown tagged templates were gaining extra indentation in nested blocks.literallinepreserves the original line breaks without adding indentation, whilededentToRootremoves any inherited indentation context.
62-67: API change from boolean to string enum is clearer.The switch from a boolean
trueto the explicit"hardline"mode improves readability and makes the intent clearer. This aligns with the updatedsimpleRehydrateDocsignature inutils.ts.src/index.ts (1)
1-11: LGTM - Clean export reorganization with backward compatibility.The deprecated alias
PrettierPluginEmbedOptionsmaintains backward compatibility while guiding users toward the newPluginEmbedOptionsname. The reorganization placing type exports before value exports follows common TypeScript conventions.tests/fixtures.spec.ts (1)
100-143: Well-structured dynamic test generation.The refactor to language-driven test suites with per-language
describeblocks and dynamic plugin loading is a clean approach that scales well as more languages are added. The@prettiercomment convention for file-specific options is intuitive.src/embedded/index.ts (1)
1-19: LGTM - Public API expansion is additive and well-organized.The new exports (
EmbeddedComment,AutocompleteStringList,fallbackIndicator) expand the public API in a backward-compatible way. Grouping related exports together improves discoverability.src/embedded/utils.ts (5)
22-39: Correct tab-width calculation for alignment.The
getAlignmentSizefunction correctly handles tab characters by snapping to the next multiple oftabWidth, which matches how tabs are typically rendered in editors and terminals.
65-91: Good on-demand indent calculation strategy.The backward-walking approach to find the most recent newline is efficient and avoids maintaining global state. The function correctly returns both
indentSizeandpreviousQuasiTextto support different alignment scenarios in the caller.
113-121: Core fix for issue #103 - SQL expression indentation.The alignment logic correctly handles two cases:
- When expression is at column 0 after a newline (
indentSize === 0 && previousQuasiText.endsWith("\n")), it usesNEGATIVE_INFINITYalignment to reset indentation- Otherwise, it applies computed alignment based on the
${}position in the templateThis ensures multi-line interpolated expressions maintain proper indentation relative to their position in the template literal.
143-191: Well-designed newline handling modes address multiple issues.The three-mode approach is clean and addresses distinct use cases:
"literalline": Preserves formatter output as-is (fixes CSS comment instability #114)"hardline": Re-indents content relative to current position (for SQL #103)false: Original behavior for backward compatibilityThe JSDoc comments clearly explain when to use each mode.
123-141: Options parameter is optional with proper defaults—no caller updates needed.The
optionsparameter inprintTemplateExpressionsis marked optional (options?: Options), and the function includes a default fallback (tabWidth = options?.tabWidth ?? 2). All 22+ callers across embedders pass onlypathand
There was a problem hiding this comment.
Pull request overview
This PR refactors the embedded language formatting logic to fix multiple indentation and formatting issues (#43, #103, #114, #120). The changes introduce more sophisticated template literal expression alignment, update the newline handling strategy for different embedded languages, and reorganize the test infrastructure to support per-language configuration.
Key changes:
- Enhanced template expression indentation calculation that respects tabWidth and properly aligns expressions based on their position in template literals
- Differentiated newline handling: 'literalline' for CSS (preserves formatter output) vs 'hardline' for SQL/markdown (re-indents from column 0)
- Refactored test infrastructure with language-specific options.json files and dynamic plugin loading
Reviewed changes
Copilot reviewed 54 out of 55 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/embedded/utils.ts | Added template expression indentation calculation logic with tabWidth support; enhanced simpleRehydrateDoc with literalline vs hardline options |
| src/embedded/css/embedder.ts | Changed newline handling from boolean true to 'literalline' to preserve CSS formatter indentation |
| src/embedded/sql/embedder.ts | Changed newline handling from boolean true to 'hardline' for proper SQL re-indentation |
| src/embedded/markdown/embedder.ts | Added dedentToRoot usage for noEmbeddedMultiLineIndentation; changed to 'hardline' newline handling |
| src/embedded/*/embedder.ts | Multiple embedders updated from boolean true to 'hardline' for newline handling |
| tests/fixtures.spec.ts | Complete test infrastructure rewrite with per-language configuration and dynamic plugin loading |
| tests/fixtures// | New test fixtures organized by language for issues #35, #45, #62, #64, #88, #103, #114, #120 |
| src/utils.ts | Changed micro-memoize import to named import; added return type annotation; modernized to Object.hasOwn |
| src/embedded/*/options.ts | Reordered imports to group type imports consistently |
| package.json | Updated dependencies including micro-memoize 4→5, prettier, and many dev dependencies |
| biome.json | Updated configuration syntax from 'include' to 'includes' with new patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/embedded/utils.ts (1)
57-91: Consider renaming for consistency.The quasi variable is now correctly named
precedingQuasi(addressing the past review), but the text variablepreviousQuasiText(lines 72, 77, 86, 90) is inconsistent. Renaming it toprecedingQuasiTextwould maintain consistency throughout the function.Apply this diff:
function getTemplateLiteralExpressionIndent( templateLiteral: TemplateLiteral, index: number, tabWidth: number, -): { indentSize: number; previousQuasiText: string } { +): { indentSize: number; precedingQuasiText: string } { let i = index; const precedingQuasi = templateLiteral.quasis[i]; - const previousQuasiText = precedingQuasi?.value.raw ?? ""; + const precedingQuasiText = precedingQuasi?.value.raw ?? ""; - if (previousQuasiText.includes("\n")) { + if (precedingQuasiText.includes("\n")) { return { - indentSize: getIndentSize(previousQuasiText, tabWidth), - previousQuasiText, + indentSize: getIndentSize(precedingQuasiText, tabWidth), + precedingQuasiText, }; } while (i-- > 0) { const q = templateLiteral.quasis[i]; if (!q) break; const text = q.value.raw; if (text.includes("\n")) { - return { indentSize: getIndentSize(text, tabWidth), previousQuasiText }; + return { indentSize: getIndentSize(text, tabWidth), precedingQuasiText }; } } - return { indentSize: 0, previousQuasiText }; + return { indentSize: 0, precedingQuasiText }; }tests/fixtures.spec.ts (1)
49-64: Consider refactoring to avoid duplicate string splitting.The content is split into lines twice: once in
findPrettierOptionsLineIndex(line 35) and again here (line 54). For large fixture files, this is inefficient.Consider refactoring to return both the line index and content from a single helper:
function findPrettierOptionsLine(content: string): { index: number; line: string } | null { const lines = content.split("\n"); for (let i = 0; i < lines.length; i++) { if (lines[i]?.match(/^\/\/\s*@prettier\s+/)) { return { index: i, line: lines[i] }; } } return null; } function parsePrettierOptions(content: string): Record<string, unknown> { const result = findPrettierOptionsLine(content); if (!result) { return {}; } const match = result.line.match(/^\/\/\s*@prettier\s+({.+})$/); if (match?.[1]) { try { return JSON.parse(match[1]); } catch { return {}; } } return {}; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/embedded/utils.ts(4 hunks)tests/fixtures.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/embedded/utils.ts (1)
src/types.ts (1)
InternalPrintFun(20-22)
🪛 GitHub Actions: Preview Release
tests/fixtures.spec.ts
[error] 3-3: TS6133: 'Plugin' is declared but its value is never read.
🪛 GitHub Check: preview-release
tests/fixtures.spec.ts
[failure] 3-3:
'Plugin' is declared but its value is never read.
🔇 Additional comments (7)
src/embedded/utils.ts (6)
22-39: LGTM: Tab alignment calculation is correct.The tab stop calculation properly rounds to the nearest multiple of
tabWidth, which is standard behavior for terminal/editor tab stops.
41-55: LGTM: Indentation extraction logic is sound.The function correctly handles both single-line strings (returns 0) and multi-line strings (extracts indentation from the last line).
123-141: LGTM: Proper options handling and parameter forwarding.The function correctly extracts
tabWidthfrom the options with a sensible default and properly passes all required parameters toprintTemplateExpression.
143-191: LGTM: Well-designed newline handling modes.The three-way
newlineHandlingparameter clearly addresses the formatting issues mentioned in #103 (SQL indentation) and #114 (CSS comment stability). The comments effectively explain when to use each mode, and the implementation correctly delegates to the appropriate Prettier utilities.
1-20: LGTM: All new imports are properly utilized.The added imports (
Options,addAlignmentToDoc,align,hardline,replaceEndOfLine) are all used in the updated function implementations.
107-118: Clarify the alignment approach for template expressions.The code uses
align(Number.NEGATIVE_INFINITY, expressionDoc)when there's a newline immediately before the expression but no indentation. While this may be intentional for handling issue #103, clarify why this specific value is used, asNumber.NEGATIVE_INFINITYis not a documented Prettier pattern. Document whether this is a custom approach, explain its purpose, and consider whether alternatives (like explicitly removing alignment or using a different sentinel value) would be clearer.tests/fixtures.spec.ts (1)
89-89: The code correctly handles plugin loading. Prettier's Node.js API accepts string plugin names in thepluginsoption and automatically resolves them viaimport(). No additional dynamic plugin loading is needed—passing strings fromloadLanguageOptions()directly toprettier.format()works as intended.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/preview-release.yml (1)
14-14: Checkout action update consistent across workflows.This uses the same
actions/checkout@v6.0.1update as other workflows. Refer to the verification request in.github/workflows/test.yml.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.github/actions/setup/action.yml(1 hunks).github/workflows/preview-release.yml(1 hunks).github/workflows/release.yml(1 hunks).github/workflows/test.yml(1 hunks).prettierignore(1 hunks)package.json(2 hunks)tests/fixtures.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/fixtures.spec.ts
🔇 Additional comments (9)
.prettierignore (1)
13-14: Good alignment with test infrastructure changes.Both additions follow standard practices and align well with the PR's test refactoring:
pnpm-lock.yamlexcludes the dependency lockfile (standard pattern)tests/**/fixtures/**/*preserves test fixture content integrity by excluding it from formatting, which is crucial given the fixture-based testing approach mentioned in the PR objectives.github/workflows/test.yml (2)
20-20: LGTM!The change to
test:cialigns with the new script in package.json (line 74) that runsvitest --run, which is appropriate for CI environments where watch mode should be disabled.
14-14: No issues found. The commit hash8e8c483db84b4bee98b60c0593521ed34d9990e8correctly corresponds tov6.0.1ofactions/checkout, and the version annotation is accurate..github/workflows/release.yml (2)
19-21: LGTM!Adding concurrency control with
cancel-in-progress: trueis a best practice for release workflows, preventing race conditions when multiple release attempts run on the same branch.
42-42: The commit hashe0145edc7d9d8679003495b11f87bd8ef63c0cbacorrectly corresponds to v1.5.3 of changesets/action. The release includes updated @actions/* and @octokit/* dependencies, switched to esbuild for bundling with a 45% file size reduction, and includes bug fixes for PR reopening, relative path handling, and subdirectory git operations. No breaking changes are present in this version.package.json (3)
74-74: LGTM!The
test:ciscript runsvitest --runfor non-watch mode execution, which is appropriate for CI environments. This change is correctly referenced in the updated CI workflows.
85-130: Dependency updates look comprehensive.The broad set of devDependency and dependency updates appears well-coordinated with the CI/test infrastructure changes. Since the previous review's concerns about Vite and Vitest were addressed (commit b8174ca), and the test:ci script ensures CI validation, these updates should be safe.
132-132: Verify PNPM 10.26.0 compatibility.The packageManager field updates from pnpm@10.4.1 to pnpm@10.26.0, a significant version jump. Verify that this version doesn't introduce breaking changes affecting lockfile format, workspace handling, or script execution by consulting the official PNPM changelog.
.github/actions/setup/action.yml (1)
8-19: Verify action versions support trusted publishing setup.The actions/setup-node v6.1.0 commit (395ad32) is correctly pinned, and the pnpm/action-setup v4.2.0 commit (41ff726) is correctly pinned. The addition of
registry-urlconfigures npm authentication, and the npm upgrade step provides support for provenance/trusted publishing features.
|
Hey @karlhorky, would you be okay to help me test if this PR fixes the mentioned issues, without introducing noteworthy regressions in your use cases? I use pkg.pr.new to release preview packages, install link here: #157 (comment) |
|
Yes, would be my pleasure, thanks for doing this! I have a medium size codebase where I can run I will take a look at this tomorrow |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/fixtures.spec.ts (1)
16-27: Silent error handling remains a concern.The function still returns an empty object when
options.jsonis missing or fails to parse, which could cause tests to pass with incorrect formatting if required plugin configurations are accidentally missing. This was previously flagged but hasn't been addressed.
🧹 Nitpick comments (6)
tests/fixtures/ruby/basic.js (1)
8-10: Verify the bare template literal is intentional.Unlike other language fixtures and the first test case in this file, this template literal lacks a variable declaration (e.g.,
const tagTest2 = ...). While syntactically valid, this is inconsistent with the established pattern.If the test runner requires variable assignments, consider applying this diff:
-/* ruby */ ` +const tagTest2 = /* ruby */ ` def add(a,b) a+b end `;However, if this format is intentional to test comment-based tag detection on bare template literals, you can disregard this suggestion.
tests/fixtures/pug/basic.js (1)
11-16: LGTM! Comment directive test case is appropriate.The fixture correctly tests the
/* pug */comment-based identification with intentional formatting issues.Optional: Consider adding test cases with JavaScript interpolations to comprehensively cover "template-literal combinations of expressions and quasis" mentioned in the PR objectives:
// Example with interpolation const name = "World"; const interpolationTest = pug` div p Hello, ${name} span(class=${className}) Dynamic `;However, this may be intentionally deferred to separate fixture files given the "basic.js" filename.
tests/fixtures/yaml/basic.js (1)
8-13: Consider adding a variable assignment for consistency.The comment-based test uses a standalone template literal without a variable assignment, unlike the tagged test above. If the test harness doesn't require this specific pattern to verify standalone template literal formatting, consider adding a variable assignment for consistency:
// YAML - Comment `/* yaml */` test (with formatting issues) -/* yaml */ ` +const commentTest = /* yaml */ ` server: host: localhost port: 3000 `;However, if the current pattern is intentional to test formatting of standalone template literals with comment markers, the existing approach is fine.
tests/fixtures/xml/basic.js (1)
1-9: Consider adding edge-case tests for indentation scenarios.Given that PR objectives include fixing indentation issues (#103, #120), you might want to add test cases that specifically cover:
- Multi-line XML with nested interpolated expressions
- Deeply nested structures to test indentation levels
- XML content that already has indentation (to ensure it's normalized)
These could be added to this file or as separate fixture files if needed.
tests/fixtures/es/basic.js (1)
6-9: Comment-activated template test looks appropriate.The standalone template literal with a preceding
/* js */comment correctly tests comment-triggered formatting. The intentionally malformed code serves as valid fixture input.However, note that this single-line embedded content doesn't exercise the multi-line indentation fixes that are central to issues #103, #114, and #120. Consider adding fixtures with multi-line embedded content to verify those scenarios are resolved.
tests/fixtures.spec.ts (1)
48-63: Consider logging parse failures for @prettier options.While silent failures are less critical for optional per-file options than for language-level configuration, malformed JSON in a
@prettiercomment could silently fail and cause confusion during test debugging. Consider adding a console warning when JSON parsing fails.Apply this diff to add a warning:
function parsePrettierOptions(content: string): Record<string, unknown> { const optionsLineIndex = findPrettierOptionsLineIndex(content); if (optionsLineIndex === -1) { return {}; } const optionsLine = content.split("\n")[optionsLineIndex] ?? ""; const match = optionsLine.match(/^\/\/\s*@prettier\s+({.+})$/); if (match?.[1]) { try { return JSON.parse(match[1]); - } catch { + } catch (error) { + console.warn(`Failed to parse @prettier options: ${optionsLine}`, error); return {}; } } return {}; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/__snapshots__/fixtures.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (41)
tests/fixtures.spec.ts(1 hunks)tests/fixtures/css/basic.js(1 hunks)tests/fixtures/es/basic.js(1 hunks)tests/fixtures/glsl/basic.js(1 hunks)tests/fixtures/glsl/options.json(1 hunks)tests/fixtures/graphql/basic.js(1 hunks)tests/fixtures/html/basic.js(1 hunks)tests/fixtures/ini/basic.js(1 hunks)tests/fixtures/ini/options.json(1 hunks)tests/fixtures/java/basic.js(1 hunks)tests/fixtures/java/options.json(1 hunks)tests/fixtures/json/basic.js(1 hunks)tests/fixtures/jsonata/basic.js(1 hunks)tests/fixtures/jsonata/options.json(1 hunks)tests/fixtures/latex/basic.js(1 hunks)tests/fixtures/latex/options.json(1 hunks)tests/fixtures/markdown/basic.js(1 hunks)tests/fixtures/nginx/basic.js(1 hunks)tests/fixtures/nginx/options.json(1 hunks)tests/fixtures/noop/basic.js(1 hunks)tests/fixtures/pegjs/basic.js(1 hunks)tests/fixtures/pegjs/options.json(1 hunks)tests/fixtures/php/basic.js(1 hunks)tests/fixtures/php/options.json(1 hunks)tests/fixtures/prisma/basic.js(1 hunks)tests/fixtures/prisma/options.json(1 hunks)tests/fixtures/properties/basic.js(1 hunks)tests/fixtures/properties/options.json(1 hunks)tests/fixtures/pug/basic.js(1 hunks)tests/fixtures/pug/options.json(1 hunks)tests/fixtures/ruby/basic.js(1 hunks)tests/fixtures/ruby/options.json(1 hunks)tests/fixtures/sh/basic.js(1 hunks)tests/fixtures/sh/options.json(1 hunks)tests/fixtures/sql/basic.js(1 hunks)tests/fixtures/toml/basic.js(1 hunks)tests/fixtures/toml/options.json(1 hunks)tests/fixtures/ts/basic.js(1 hunks)tests/fixtures/xml/basic.js(1 hunks)tests/fixtures/xml/options.json(1 hunks)tests/fixtures/yaml/basic.js(1 hunks)
✅ Files skipped from review due to trivial changes (17)
- tests/fixtures/toml/options.json
- tests/fixtures/sh/options.json
- tests/fixtures/ruby/options.json
- tests/fixtures/glsl/options.json
- tests/fixtures/sh/basic.js
- tests/fixtures/css/basic.js
- tests/fixtures/glsl/basic.js
- tests/fixtures/pegjs/options.json
- tests/fixtures/html/basic.js
- tests/fixtures/ini/options.json
- tests/fixtures/java/options.json
- tests/fixtures/ts/basic.js
- tests/fixtures/pegjs/basic.js
- tests/fixtures/nginx/basic.js
- tests/fixtures/nginx/options.json
- tests/fixtures/json/basic.js
- tests/fixtures/prisma/options.json
🧰 Additional context used
🧬 Code graph analysis (5)
tests/fixtures/properties/basic.js (16)
tests/fixtures/css/basic.js (1)
tagTest(2-4)tests/fixtures/es/basic.js (1)
tagTest(2-4)tests/fixtures/glsl/basic.js (1)
tagTest(2-4)tests/fixtures/graphql/basic.js (1)
tagTest(2-4)tests/fixtures/html/basic.js (1)
tagTest(2-4)tests/fixtures/ini/basic.js (1)
tagTest(2-7)tests/fixtures/java/basic.js (1)
tagTest(2-4)tests/fixtures/json/basic.js (1)
tagTest(2-4)tests/fixtures/jsonata/basic.js (1)
tagTest(2-4)tests/fixtures/latex/basic.js (1)
tagTest(2-4)tests/fixtures/markdown/basic.js (1)
tagTest(2-7)tests/fixtures/nginx/basic.js (1)
tagTest(2-4)tests/fixtures/noop/basic.js (1)
tagTest(2-4)tests/fixtures/pegjs/basic.js (1)
tagTest(2-5)tests/fixtures/php/basic.js (1)
tagTest(2-4)tests/fixtures/prisma/basic.js (1)
tagTest(2-4)
tests/fixtures/xml/basic.js (16)
tests/fixtures/css/basic.js (1)
tagTest(2-4)tests/fixtures/es/basic.js (1)
tagTest(2-4)tests/fixtures/glsl/basic.js (1)
tagTest(2-4)tests/fixtures/graphql/basic.js (1)
tagTest(2-4)tests/fixtures/html/basic.js (1)
tagTest(2-4)tests/fixtures/ini/basic.js (1)
tagTest(2-7)tests/fixtures/java/basic.js (1)
tagTest(2-4)tests/fixtures/json/basic.js (1)
tagTest(2-4)tests/fixtures/jsonata/basic.js (1)
tagTest(2-4)tests/fixtures/latex/basic.js (1)
tagTest(2-4)tests/fixtures/markdown/basic.js (1)
tagTest(2-7)tests/fixtures/nginx/basic.js (1)
tagTest(2-4)tests/fixtures/noop/basic.js (1)
tagTest(2-4)tests/fixtures/pegjs/basic.js (1)
tagTest(2-5)tests/fixtures/php/basic.js (1)
tagTest(2-4)tests/fixtures/prisma/basic.js (1)
tagTest(2-4)
tests/fixtures/yaml/basic.js (16)
tests/fixtures/css/basic.js (1)
tagTest(2-4)tests/fixtures/es/basic.js (1)
tagTest(2-4)tests/fixtures/glsl/basic.js (1)
tagTest(2-4)tests/fixtures/graphql/basic.js (1)
tagTest(2-4)tests/fixtures/html/basic.js (1)
tagTest(2-4)tests/fixtures/ini/basic.js (1)
tagTest(2-7)tests/fixtures/java/basic.js (1)
tagTest(2-4)tests/fixtures/json/basic.js (1)
tagTest(2-4)tests/fixtures/jsonata/basic.js (1)
tagTest(2-4)tests/fixtures/latex/basic.js (1)
tagTest(2-4)tests/fixtures/markdown/basic.js (1)
tagTest(2-7)tests/fixtures/nginx/basic.js (1)
tagTest(2-4)tests/fixtures/noop/basic.js (1)
tagTest(2-4)tests/fixtures/pegjs/basic.js (1)
tagTest(2-5)tests/fixtures/php/basic.js (1)
tagTest(2-4)tests/fixtures/prisma/basic.js (1)
tagTest(2-4)
tests/fixtures/jsonata/basic.js (1)
tests/fixtures/json/basic.js (1)
tagTest(2-4)
tests/fixtures/ruby/basic.js (16)
tests/fixtures/css/basic.js (1)
tagTest(2-4)tests/fixtures/es/basic.js (1)
tagTest(2-4)tests/fixtures/glsl/basic.js (1)
tagTest(2-4)tests/fixtures/graphql/basic.js (1)
tagTest(2-4)tests/fixtures/html/basic.js (1)
tagTest(2-4)tests/fixtures/ini/basic.js (1)
tagTest(2-7)tests/fixtures/java/basic.js (1)
tagTest(2-4)tests/fixtures/json/basic.js (1)
tagTest(2-4)tests/fixtures/jsonata/basic.js (1)
tagTest(2-4)tests/fixtures/latex/basic.js (1)
tagTest(2-4)tests/fixtures/markdown/basic.js (1)
tagTest(2-7)tests/fixtures/nginx/basic.js (1)
tagTest(2-4)tests/fixtures/noop/basic.js (1)
tagTest(2-4)tests/fixtures/pegjs/basic.js (1)
tagTest(2-5)tests/fixtures/php/basic.js (1)
tagTest(2-4)tests/fixtures/prisma/basic.js (1)
tagTest(2-4)
🔇 Additional comments (37)
tests/fixtures/noop/basic.js (2)
1-4: LGTM! Clear test case for tag-based noop behavior.The test case correctly demonstrates the expected noop formatting behavior using a tagged template literal. The multiple spaces provide a good verification point that content preservation is working as intended.
6-9: LGTM! Valid test case for comment-based noop behavior.The standalone template literal expression preceded by a
/* noop */comment correctly tests the comment-based noop trigger mechanism. The multiple spaces will verify that the formatter skips processing when this comment is present.tests/fixtures/pug/options.json (1)
1-3: LGTM!The configuration correctly references the official
@prettier/plugin-pugpackage, which is actively maintained (version 3.4.2 published recently). This minimal configuration is appropriate for test fixtures and matches the recommended setup in the official documentation.tests/fixtures/ruby/basic.js (1)
1-5: LGTM!The first test case follows the established pattern from other language fixtures and appropriately includes intentional formatting issues to test the formatter's behavior.
tests/fixtures/pug/basic.js (1)
1-9: LGTM! Well-designed test fixture for tagged template indentation.The intentional formatting inconsistencies (title at 6 spaces, p at 5 spaces) properly exercise the formatter's indentation normalization, aligning with the PR's goal to fix indentation issues in embedded templates.
tests/fixtures/toml/basic.js (2)
1-6: LGTM!The tagged template fixture correctly demonstrates TOML content with formatting issues (missing spaces around
=operators) to verify the formatter's behavior.
8-13: No action needed—comment-based embedding works as intended.The unassigned template literal is the correct pattern for comment-based embedding (
/* toml */ \...`), distinct from tag-based embedding (e.g.,const tagTest = toml`...``). This pattern is consistent across all fixture files and reflects the plugin's design supporting both embedding methods.tests/fixtures/java/basic.js (2)
1-4: LGTM! Standard test fixture pattern.The tagged template with intentionally malformed Java code is appropriate for testing the formatter's ability to fix formatting issues.
6-9: Verify the standalone template literal pattern is intentional.The template literal on lines 7-9 is not assigned to a variable, creating a standalone expression. This differs from the first example and results in the template's value being discarded.
If this is testing comment-based language hints (
/* java */) for unassigned template literals, please confirm this pattern is documented and expected. Otherwise, consider adding an assignment similar to the first example.#!/bin/bash # Description: Check if comment-based language hints are documented and verify usage patterns in other fixture files. # Search for documentation of comment-based language hints rg -n -C3 'java.*\*/' --type=md # Search for similar patterns in other fixture files rg -n '\/\*\s+\w+\s+\*\/\s*`' tests/fixtures/tests/fixtures/properties/options.json (1)
1-3: LGTM!The plugin configuration is correct and follows the standard Prettier options format for test fixtures.
tests/fixtures/properties/basic.js (2)
1-6: LGTM!The tagged template test follows the established pattern from other language fixtures and includes appropriate formatting issues (inconsistent spacing) for testing.
8-12: The current code is correct and consistent with all other language fixtures in the repository.Examining the pattern across fixture files (markdown, es, ini, graphql, pegjs, html) shows that all fixtures use the same pattern: the tag-based syntax uses
const tagTest, while the comment-based syntax deliberately uses an unassigned template literal. The properties/basic.js file follows this established pattern correctly and requires no changes.Likely an incorrect or invalid review comment.
tests/fixtures/yaml/basic.js (1)
1-6: LGTM! Consistent with established fixture pattern.The tagged template test follows the same structure as other language fixtures (markdown, css, json, etc.), using an undefined identifier as the template tag. The YAML content is syntactically valid, and the intentional formatting issues (extra spaces) appropriately test the formatter's normalization capabilities.
tests/fixtures/sql/basic.js (2)
6-9: LGTM!The comment-based SQL embedding test case is appropriately structured as a fixture. The intentionally poor formatting will help verify the plugin's formatting capabilities.
2-2: No action needed. Thesqltag is intentionally undefined—these fixture files are test input for the prettier plugin's formatting logic, not executable JavaScript. The plugin reads them as plain text to identify and format embedded language blocks. This undefined-identifier pattern is used consistently across all language fixtures (css, graphql, html, etc.) in the project.Likely an incorrect or invalid review comment.
tests/fixtures/markdown/basic.js (2)
1-7: LGTM! Well-structured test fixture.The first test case correctly uses a tagged template literal with intentional formatting issues (inconsistent list markers and spacing) to verify Markdown normalization behavior.
9-14: No changes needed. The loose template literal is an intentional design pattern used consistently across all fixture files (CSS, ES, GraphQL, Markdown, etc.) to test both tag-based (md\...`) and comment-based (/* markdown */ `...``) embedded code markers. The test runner reads the entire file content and passes it to Prettier, formatting both test cases together.tests/fixtures/latex/basic.js (2)
6-9: The standalone template literal with the/* latex */comment marker is intentional. Every fixture file in the test suite follows this pattern to test both tag-function embedding (lines 2-4) and comment-marker embedding (lines 7-9). The standalone expression is a valid JavaScript expression statement that tests the plugin's ability to recognize and format embedded content via comment markers rather than tag functions.
1-4: No action required. These fixture files are test input data meant to be parsed and formatted by the plugin, not executed as JavaScript. Undefined tag identifiers are intentional across all fixture files in the repository and are part of the test structure.tests/fixtures/latex/options.json (1)
1-3: The "prettier-plugin-latex" package name is correct and exists on npm with multiple versions available (latest: 2.0.1). No changes needed.tests/fixtures/xml/basic.js (2)
1-4: LGTM! Consistent test pattern for XML tagged template.The first test case follows the established pattern seen in other language fixtures. The intentionally unformatted XML will effectively test the formatter's ability to handle tagged template literals.
6-9: LGTM! Good coverage of comment-style annotation.The second test case appropriately tests the
/* xml */comment annotation syntax, providing coverage for both identification methods supported by the plugin.tests/fixtures/xml/options.json (1)
1-3: Configuration is correct.The
@prettier/plugin-xmlpackage exists and is compatible with the project's Prettier version (^3.7.4). The configuration file is valid.tests/fixtures/es/basic.js (1)
1-4: The fixture is correctly structured and properly tested with Vitest snapshots. The undefinedjstag is intentional—the fixture tests the formatter's behavior with undefined tagged templates. Expected output is captured in the snapshot attests/__snapshots__/fixtures.spec.ts.snap, not as separate files. For PR-specific issues (#103, #114, #120), dedicated fixtures exist in their respective language directories:sql/issue-103.js,css/issue-114.js, andmarkdown/issue-120.js. Thebasic.jsfixture appropriately covers basic formatting scenarios.tests/fixtures/php/basic.js (1)
1-9: LGTM! Test fixtures are well-structured.The test fixtures correctly demonstrate both tag-based (
php) and comment-based (/* php */) embedding patterns. The intentionally poorly formatted PHP code is appropriate for testing the formatter's capabilities.tests/fixtures/graphql/basic.js (1)
1-9: LGTM! Test fixtures follow the expected pattern.The GraphQL test fixtures correctly demonstrate both tag-based and comment-based embedding. The intentionally compressed GraphQL queries (missing spaces, newlines) are appropriate for verifying the formatter's ability to restore proper formatting.
tests/fixtures/jsonata/basic.js (1)
1-9: LGTM! Test fixtures are consistent with the pattern.The JSONata test fixtures follow the same pattern as other language fixtures, demonstrating both tag-based and comment-based embedding. The intentionally compressed JSONata expressions are appropriate for testing formatting capabilities.
tests/fixtures/prisma/basic.js (1)
1-9: LGTM! Prisma test fixtures are well-formed.The Prisma schema test fixtures correctly follow the established pattern for embedded language testing. The intentionally compressed schema definitions (missing spacing and newlines) are appropriate for verifying the formatter's capabilities.
tests/fixtures/ini/basic.js (1)
1-14: LGTM! INI test fixtures demonstrate spacing issues appropriately.The INI test fixtures correctly showcase inconsistent spacing around the
=delimiter (lines 5-6, 13), which is the intended formatting issue to be tested. Both tag-based and comment-based embedding patterns are properly demonstrated.tests/fixtures.spec.ts (6)
8-11: LGTM! Interface definition is appropriate.The
LanguageOptionsinterface correctly models the structure needed for language-specific configuration, with an optionalpluginsarray and flexible support for additional Prettier options via the index signature.
33-41: LGTM! Line-finding logic is correct.The function correctly identifies the
@prettieroptions comment line using an appropriate regex pattern. The sentinel value of-1for "not found" is a standard and clear convention.
68-75: LGTM! Content stripping logic is correct.The function correctly removes the
@prettieroptions line from the content before formatting, ensuring that the comment doesn't interfere with the test fixture's embedded code. The logic properly handles both cases (with and without options line).
77-81: LGTM! Fixture directory discovery is well-structured.The code correctly uses
import.meta.dirnameandglobSyncto discover language-specific fixture directories. TheonlyDirectories: trueoption ensures clean directory-based organization.
83-101: LGTM! Language-level test organization is well-designed.The structure correctly organizes tests by language directory and loads language-specific options once per language (line 88). The
beforeAllhook appropriately mutes Pug logs at the language level, avoiding unnecessary noise during test execution.
103-127: LGTM! Test generation logic is correct and efficient.The test structure properly:
- Skips configuration files (line 105-107)
- Loads per-file options and strips the options comment before formatting (lines 113-114)
- Composes the plugins array once per test using the language-level configuration (line 119)
- Uses snapshot testing appropriately for formatter output verification
The plugin loading strategy is efficient: plugin module names are read from
options.jsononce per language (line 88) and passed toprettier.format(line 119), where Prettier handles the actual module loading with Node's import caching. This avoids the performance concerns mentioned in previous reviews.tests/fixtures/jsonata/options.json (1)
2-2: Verify that the JSONata plugin is properly configured.Ensure that
@stedi/prettier-plugin-jsonatais included in the project's dependencies and that the plugin is compatible with the Prettier version in use.tests/fixtures/php/options.json (1)
2-2: The versions specified are accurate:@prettier/plugin-php^0.24.0 and Prettier ^3.7.4 are both declared in devDependencies. The plugin is properly configured in the options.json file and the test fixtures use this configuration without apparent issues. Compatibility verification would require checking the plugin's package metadata on npm, which cannot be accessed in the current environment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
21-21: Consider pinning ruby/setup-ruby to a commit SHA or minor version.Using
@v1allows the action to float to any v1.x.x release, which could introduce unexpected changes. For better reproducibility and security, consider pinning to a specific commit SHA (like the checkout action above) or at minimum a minor version (e.g.,@v1.203.0).🔎 Example with SHA pinning:
- uses: ruby/setup-ruby@v1 + uses: ruby/setup-ruby@0cdeaaa8f1b6e2f62ab072ff7ece6d91cf95339d # v1.203.0Note: Replace the SHA with the actual commit hash from the version you want to pin.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(1 hunks)
🔇 Additional comments (3)
.github/workflows/test.yml (3)
27-27: LGTM: Test command updated to use CI-specific script.The change from
pnpm -s testtopnpm -s test:cialigns with the new test:ci script mentioned in the AI summary context. This separation of CI-specific test configuration is a good practice.
23-23: Ruby 3.4 is stable and supported. The current stable version is 3.4.8, and ruby/setup-ruby supports all versions from 2.3.0 until 4.0.0-preview2, which includes Ruby 3.4. No action needed.Likely an incorrect or invalid review comment.
14-14: The actions/checkout version and SHA are correct.The commit SHA 8e8c483db84b4bee98b60c0593521ed34d9990e8 corresponds to v6.0.1, which is a valid release. Pinning to a specific commit SHA is a security best practice for GitHub Actions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.changeset/icy-impalas-laugh.md (1)
5-5: Consider expanding the changeset description to reflect all fixes.The description mentions only XML and Prisma, but the PR addresses multiple issues (#43, #103, #114, #120) spanning SQL indentation, CSS comments stability, tagged templates indentation, and test coverage. A more comprehensive description would help users understand the scope of fixes in this release.
For example, you might expand to something like: "Fix embedded language formatting behavior including SQL indentation, CSS comments stability, and tagged templates indentation."
src/embedded/xml/parser.ts (1)
24-158: Consider extracting node-type handlers to reduce function complexity.The 134-line
simplifyCSTfunction handles 10+ XML node types in a single switch statement. While each case is straightforward, extracting them into separate handler functions (e.g.,simplifyDocument,simplifyProlog) would improve maintainability and testability.Example refactoring approach
function simplifyDocument(node: CstNode): SimplifiedNode { const children = node.children; return { name: node.name, location: node.location, prolog: simplifyOne(children.prolog), docTypeDecl: simplifyOne(children.docTypeDecl), element: simplifyOne(children.element), misc: (children.misc ?? []) .filter( (child): child is CstNode => isCstNode(child) && !child.children.SEA_WS, ) .map(simplifyCST), }; } // Then in simplifyCST: case "document": return simplifyDocument(node);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/__snapshots__/fixtures.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
.changeset/icy-impalas-laugh.md(1 hunks)src/embedded/prisma/embedder.ts(1 hunks)src/embedded/xml/parser.ts(4 hunks)tests/fixtures/html/options.json(1 hunks)tests/fixtures/prisma/basic.js(1 hunks)tests/fixtures/xml/options.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/fixtures/xml/options.json
- tests/fixtures/prisma/basic.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/embedded/prisma/embedder.ts (1)
src/embedded/utils.ts (1)
simpleRehydrateDoc(143-191)
src/embedded/xml/parser.ts (1)
src/embedded/xml/options.ts (1)
options(27-64)
🔇 Additional comments (5)
tests/fixtures/html/options.json (1)
1-4: LGTM! Clean test fixture configuration.The HTML test fixture options are well-configured. The empty
pluginsarray is appropriate since the test framework will inject the plugin under test, andhtmlWhitespaceSensitivity: "ignore"is a sensible default for testing embedded HTML formatting without strict whitespace constraints.src/embedded/prisma/embedder.ts (1)
52-57: LGTM! Newline handling mode is now explicit.The change from a boolean flag to the explicit
"hardline"mode improves API clarity and aligns with the refactoredsimpleRehydrateDocsignature. The"hardline"mode correctly adds indentation to embedded Prisma content based on the current indent level.src/embedded/xml/parser.ts (3)
222-321: LGTM! Robust parser implementation with comprehensive error handling.The parser correctly:
- Transforms CST to simplified AST format via
simplifyCST- Handles lexical and parse errors with appropriate recovery strategies
- Supports fragment recovery for multi-element cases
- Provides safe fallbacks in
locStart/locEndwhen location data is missingThe change from
Parser<CstNode>toParser<SimplifiedNode>is a breaking API change but aligns with the PR's refactoring objectives.
344-348: LGTM! Type guard correctly distinguishes CstNode from IToken.The three-property check (
"name","children","location") accurately identifies Chevrotain CST nodes within theCstElementunion type.
49-55: Clarify the misc node filter logic to match actual behavior.The comment states "Filter out misc nodes that only contain SEA_WS" but the condition
!child.children.SEA_WSfilters out ALL misc nodes that have aSEA_WSproperty, regardless of whether they also containCommentorPROCESSING_INSTRUCTION. A more accurate comment would reflect that the filter removes any misc node containing whitespace, not just those with only whitespace.
|
@Sec-ant looks pretty good to me! 👍 I didn't see any regressions in our database queries, and these indentation problems were fixed: 
const companyPreviewStyles = css`
/*
Minimum column size of 0 required to prevent long
words from increasing the column size
https://css-tricks.com/preventing-a-grid-blowout/
*/
grid-template-columns: 115px minmax(0, 1fr);
`;After 1st format (identical - no longer any increased indentation inside comment 🎉): const companyPreviewStyles = css`
/*
Minimum column size of 0 required to prevent long
words from increasing the column size
https://css-tricks.com/preventing-a-grid-blowout/
*/
grid-template-columns: 115px minmax(0, 1fr);
`; |
|
This patch is shipped in v0.5.1 |
Fixes #43
Fixes #103
Fixes #114
Fixes #120
Should also fixes prettier/plugin-xml#810
The code is mainly generated by Claude Opus 4.5, I give it some coding style and file organization suggestions.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
CI
✏️ Tip: You can customize this high-level summary in your review settings.