From cf8fbf316d9d8a83a30c5ceae326cc700d0ab318 Mon Sep 17 00:00:00 2001 From: Marco Roth Date: Sun, 16 Nov 2025 00:20:24 -0800 Subject: [PATCH 1/3] Formatter: Improve element and blank line spacing --- javascript/packages/core/src/ast-utils.ts | 23 ++- .../packages/formatter/src/format-printer.ts | 157 ++++++++++++++++-- .../test/document-formatting.test.ts | 2 - .../erb-formatter-additional.test.ts | 2 +- .../erb-formatter-compat.test.ts | 4 +- .../erb-formatter-fixtures.test.ts | 6 +- .../formatter/test/erb-grouping.test.ts | 98 +++++++++++ .../formatter/test/erb/comment.test.ts | 6 - .../packages/formatter/test/erb/erb.test.ts | 23 +-- .../packages/formatter/test/erb/if.test.ts | 3 + .../formatter/test/erb/unless.test.ts | 6 + .../test/erb/whitespace-formatting.test.ts | 1 - .../formatter/test/frontmatter.test.ts | 1 - .../packages/formatter/test/head.test.ts | 76 +++++++++ .../formatter/test/html/comments.test.ts | 1 - .../test/html/quote-normalization.test.ts | 7 +- .../formatter/test/html/text-content.test.ts | 1 + .../test/html/unicode-characters.test.ts | 9 +- .../formatter/test/multiline-spacing.test.ts | 135 +++++++++++++++ .../packages/formatter/test/spacing.test.ts | 27 +-- .../test/xml-erb-integration.test.ts | 6 + .../packages/formatter/test/xml/xml.test.ts | 8 + javascript/packages/vscode/package.json | 2 +- 23 files changed, 525 insertions(+), 79 deletions(-) create mode 100644 javascript/packages/formatter/test/erb-grouping.test.ts create mode 100644 javascript/packages/formatter/test/head.test.ts create mode 100644 javascript/packages/formatter/test/multiline-spacing.test.ts diff --git a/javascript/packages/core/src/ast-utils.ts b/javascript/packages/core/src/ast-utils.ts index 6c844a4cd..372f8e14d 100644 --- a/javascript/packages/core/src/ast-utils.ts +++ b/javascript/packages/core/src/ast-utils.ts @@ -19,11 +19,11 @@ import { } from "./nodes.js" import { - isNode, isAnyOf, isLiteralNode, isERBNode, isERBContentNode, + isHTMLCommentNode, areAllOfType, filterLiteralNodes } from "./node-type-guards.js" @@ -37,6 +37,12 @@ export type ERBOutputNode = ERBNode & { } } +export type ERBCommentNode = ERBNode & { + tag_opening: { + value: "<%#" + } +} + /** * Checks if a node is an ERB output node (generates content: <%= %> or <%== %>) */ @@ -47,6 +53,17 @@ export function isERBOutputNode(node: Node): node is ERBOutputNode { return ["<%=", "<%=="].includes(node.tag_opening?.value) } +/** + * Checks if a node is a ERB comment node (control flow: <%# %>) + */ +export function isERBCommentNode(node: Node): node is ERBCommentNode { + if (!isERBNode(node)) return false + if (!node.tag_opening?.value) return false + + return node.tag_opening?.value === "<%#" || (node.tag_opening?.value !== "<%#" && (node.content?.value || "").trimStart().startsWith("#")) +} + + /** * Checks if a node is a non-output ERB node (control flow: <% %>) */ @@ -200,8 +217,8 @@ export function getTagName(node: HTMLElementNode | HTMLOpenTagNode | HTMLCloseTa /** * Check if a node is a comment (HTML comment or ERB comment) */ -export function isCommentNode(node: Node): boolean { - return isNode(node, HTMLCommentNode) || (isERBNode(node) && !isERBControlFlowNode(node)) +export function isCommentNode(node: Node): node is HTMLCommentNode | ERBCommentNode { + return isHTMLCommentNode(node) || isERBCommentNode(node) } /** diff --git a/javascript/packages/formatter/src/format-printer.ts b/javascript/packages/formatter/src/format-printer.ts index 6dd97ec57..bc7c5d2e4 100644 --- a/javascript/packages/formatter/src/format-printer.ts +++ b/javascript/packages/formatter/src/format-printer.ts @@ -13,6 +13,8 @@ import { isERBNode, isCommentNode, isERBControlFlowNode, + isERBCommentNode, + isERBOutputNode, filterNodes, } from "@herb-tools/core" @@ -288,6 +290,86 @@ export class FormatPrinter extends Printer { return nodes.filter(child => isNoneOf(child, HTMLAttributeNode, WhitespaceNode)) } + + /** + * Determine if an HTML element or ERB block would format to multiple lines + * Used for spacing multiline elements + * + * We render the node and check if the output contains newlines. + * This is the most accurate and reliable way to determine multiline status. + */ + private isMultilineElement(node: Node): boolean { + try { + return this.capture(() => this.visit(node)).join("\n").trim().includes("\n") + } catch (e) { + return false + } + } + + /** + * Get a grouping key for a node (tag name for HTML, ERB type for ERB) + */ + private getGroupingKey(node: Node): string | null { + if (isNode(node, HTMLElementNode)) { + return getTagName(node) + } + + if (isERBOutputNode(node)) return "erb-output" + if (isERBCommentNode(node)) return "erb-comment" + if (isERBNode(node)) return "erb-code" + + return null + } + + /** + * Detect groups of consecutive same-tag/same-type single-line elements + * Returns a map of index -> group info for efficient lookup + */ + private detectTagGroups(siblings: Node[]): Map { + const groupMap = new Map() + const meaningfulNodes: Array<{ index: number; groupKey: string }> = [] + + for (let i = 0; i < siblings.length; i++) { + const node = siblings[i] + + if (!this.isMultilineElement(node)) { + const groupKey = this.getGroupingKey(node) + + if (groupKey) { + meaningfulNodes.push({ index: i, groupKey }) + } + } + } + + let groupStart = 0 + + while (groupStart < meaningfulNodes.length) { + const startGroupKey = meaningfulNodes[groupStart].groupKey + let groupEnd = groupStart + + while (groupEnd + 1 < meaningfulNodes.length && meaningfulNodes[groupEnd + 1].groupKey === startGroupKey) { + groupEnd++ + } + + if (groupEnd > groupStart) { + const groupStartIndex = meaningfulNodes[groupStart].index + const groupEndIndex = meaningfulNodes[groupEnd].index + + for (let i = groupStart; i <= groupEnd; i++) { + groupMap.set(meaningfulNodes[i].index, { + tagName: startGroupKey, + groupStart: groupStartIndex, + groupEnd: groupEndIndex + }) + } + } + + groupStart = groupEnd + 1 + } + + return groupMap + } + /** * Determine if spacing should be added between sibling elements * @@ -309,16 +391,57 @@ export class FormatPrinter extends Printer { currentIndex: number, hasExistingSpacing: boolean ): boolean { + const currentNode = siblings[currentIndex] + if (hasExistingSpacing) { return true } + const previousMeaningfulIndex = findPreviousMeaningfulSibling(siblings, currentIndex) + + if (previousMeaningfulIndex !== -1) { + const previousNode = siblings[previousMeaningfulIndex] + + if (isNode(previousNode, XMLDeclarationNode) || isNode(previousNode, HTMLDoctypeNode)) { + return true + } + } + + const isCurrentComment = isCommentNode(currentNode) + + if (previousMeaningfulIndex !== -1) { + const previousNode = siblings[previousMeaningfulIndex] + const isPreviousComment = isCommentNode(previousNode) + + if (isPreviousComment && !isCurrentComment && (isNode(currentNode, HTMLElementNode) || isERBNode(currentNode))) { + const isPreviousMultiline = this.isMultilineElement(previousNode) + const isCurrentMultiline = this.isMultilineElement(currentNode) + + if (isPreviousMultiline && isCurrentMultiline) { + return true + } + + return false + } + + if (isPreviousComment && isCurrentComment) { + return false + } + } + const hasMixedContent = siblings.some(child => isNode(child, HTMLTextNode) && child.content.trim() !== "") if (hasMixedContent) { return false } + const isCurrentMultiline = this.isMultilineElement(currentNode) + const isPreviousMultiline = previousMeaningfulIndex !== -1 && this.isMultilineElement(siblings[previousMeaningfulIndex]) + + if (isCurrentMultiline || isPreviousMultiline) { + return true + } + const meaningfulSiblings = siblings.filter(child => isNonWhitespaceNode(child)) if (meaningfulSiblings.length < SPACING_THRESHOLD) { @@ -337,20 +460,17 @@ export class FormatPrinter extends Printer { return false } - const currentNode = siblings[currentIndex] - const previousMeaningfulIndex = findPreviousMeaningfulSibling(siblings, currentIndex) - const isCurrentComment = isCommentNode(currentNode) - if (previousMeaningfulIndex !== -1) { - const previousNode = siblings[previousMeaningfulIndex] - const isPreviousComment = isCommentNode(previousNode) + const tagGroups = this.detectTagGroups(siblings) + const currentGroup = tagGroups.get(currentIndex) + const previousGroup = tagGroups.get(previousMeaningfulIndex) - if (isPreviousComment && !isCurrentComment && (isNode(currentNode, HTMLElementNode) || isERBNode(currentNode))) { + if (currentGroup && previousGroup && currentGroup.groupStart === previousGroup.groupStart && currentGroup.groupEnd === previousGroup.groupEnd) { return false } - if (isPreviousComment && isCurrentComment) { - return false + if (previousGroup && currentGroup && previousGroup.groupEnd === previousMeaningfulIndex && currentGroup.groupStart === currentIndex) { + return true } } @@ -676,7 +796,21 @@ export class FormatPrinter extends Printer { } if (isNonWhitespaceNode(child) && lastWasMeaningful && !hasHandledSpacing) { - this.push("") + const previousNode = i > 0 ? children[i - 1] : null + const hasExistingSpacing = !!(previousNode && isNode(previousNode, HTMLTextNode) && + previousNode.content.trim() === "" && + (previousNode.content.includes('\n\n') || previousNode.content.split('\n').length > 2)) + + const shouldAddSpacing = this.shouldAddSpacingBetweenSiblings( + null, + children, + i, + hasExistingSpacing + ) + + if (shouldAddSpacing) { + this.push("") + } } this.visit(child) @@ -1189,8 +1323,7 @@ export class FormatPrinter extends Printer { } visitERBContentNode(node: ERBContentNode) { - // TODO: this feels hacky - if (node.tag_opening?.value === "<%#") { + if (isERBCommentNode(node)) { this.visitERBCommentNode(node) } else { this.printERBNode(node) diff --git a/javascript/packages/formatter/test/document-formatting.test.ts b/javascript/packages/formatter/test/document-formatting.test.ts index 1f7d4c633..09e1a89db 100644 --- a/javascript/packages/formatter/test/document-formatting.test.ts +++ b/javascript/packages/formatter/test/document-formatting.test.ts @@ -116,7 +116,6 @@ describe("Document-level formatting", () => { const result = formatter.format(source) expect(result).toEqual(dedent` <% title = "Test" %> -
<%= title %>
`) }) @@ -147,7 +146,6 @@ describe("Document-level formatting", () => { const result = formatter.format(source) expect(result).toEqual(dedent` <% page_title = "User Profile" %> - <% user_data = { name: "John", age: 30 } %> diff --git a/javascript/packages/formatter/test/erb-formatter/erb-formatter-additional.test.ts b/javascript/packages/formatter/test/erb-formatter/erb-formatter-additional.test.ts index d9f14add6..8bc865623 100644 --- a/javascript/packages/formatter/test/erb-formatter/erb-formatter-additional.test.ts +++ b/javascript/packages/formatter/test/erb-formatter/erb-formatter-additional.test.ts @@ -85,7 +85,6 @@ describe("ERB Formatter Additional Tests", () => { expect(result).toBe(dedent` <%- vite_client_tag %> - <%= vite_typescript_tag "application", "data-turbo-track": "reload", defer: true %> <%= stylesheet_link_tag "tailwind", @@ -159,6 +158,7 @@ describe("ERB Formatter Additional Tests", () => {
Short text +

In the event we decide to issue a refund, we will reimburse you no later than fourteen (14) days from the date on which we make that diff --git a/javascript/packages/formatter/test/erb-formatter/erb-formatter-compat.test.ts b/javascript/packages/formatter/test/erb-formatter/erb-formatter-compat.test.ts index d71587e39..8582ceb01 100644 --- a/javascript/packages/formatter/test/erb-formatter/erb-formatter-compat.test.ts +++ b/javascript/packages/formatter/test/erb-formatter/erb-formatter-compat.test.ts @@ -120,7 +120,6 @@ describe("ERB Formatter Compatibility Tests", () => { expect(result).toEqual(dedent` <% if eeee then "b" else c end %> - <% if eeee then a else c end %> `) }) @@ -147,7 +146,6 @@ describe("ERB Formatter Compatibility Tests", () => { expect(result).toEqual(dedent`

>
-
>
`) }) @@ -328,7 +326,6 @@ describe("ERB Formatter Compatibility Tests", () => { expect(result).toEqual(dedent` <%# This is a comment %> -
Content
<% # Another comment style %> @@ -371,6 +368,7 @@ describe("ERB Formatter Compatibility Tests", () => { <% categories.each do |category| %>

<%= category.name %>

+ <% category.items.each do |item| %>
<%= item.name %>
<% end %> diff --git a/javascript/packages/formatter/test/erb-formatter/erb-formatter-fixtures.test.ts b/javascript/packages/formatter/test/erb-formatter/erb-formatter-fixtures.test.ts index 0c4ab6a8a..717e4b2ed 100644 --- a/javascript/packages/formatter/test/erb-formatter/erb-formatter-fixtures.test.ts +++ b/javascript/packages/formatter/test/erb-formatter/erb-formatter-fixtures.test.ts @@ -167,6 +167,7 @@ describe("ERB Formatter Fixture Tests", () => { <% when 'admin', 'moderator' %>

Admin Controls

+ <% if user.permissions.include?('delete') %> <% end %> @@ -314,13 +315,11 @@ describe("ERB Formatter Fixture Tests", () => { expect(result).toBe(dedent` <% if eeee then "b" else c end %> - <% if eeee then a else c end %> <% if longlonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglonglong then a else c end %>
>
-
>
{ expect(result).toBe(dedent`

Welcome

+ <% if user.present? %>

Hello <%= user.name %>!

<% else %> @@ -409,6 +409,7 @@ describe("ERB Formatter Fixture Tests", () => { <%= page_title %> + <% content_for :navigation do %>