-
Notifications
You must be signed in to change notification settings - Fork 56
fix: preserve leading empty lines in code cells (if any) during formatting #953
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
|
|
||
| import { | ||
| commands, | ||
| EndOfLine, | ||
| FormattingOptions, | ||
| Position, | ||
| Range, | ||
|
|
@@ -311,14 +312,51 @@ async function formatBlock( | |
|
|
||
| // Create virtual document containing only the code portion of the block | ||
| // so the formatter never sees the option directives. | ||
| const codeLines = blockLines.slice(optionLines); | ||
| // | ||
| // Leading empty lines are also hidden: formatters like Air (R) strip empty | ||
| // lines at position 0 of a file, which would delete them from the cell. | ||
| // We track the count so formatter edits can be shifted past them and any | ||
| // excess collapsed to one (see normalizeEdit below). | ||
| let leadingEmptyLines = 0; | ||
| for (let i = optionLines; i < blockLines.length; i++) { | ||
| if (blockLines[i].trim() === "") { | ||
| leadingEmptyLines++; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| const codeLines = blockLines.slice(optionLines + leadingEmptyLines); | ||
|
|
||
| const blockRange = new Range( | ||
| new Position(block.range.start.line, block.range.start.character), | ||
| new Position(block.range.end.line, block.range.end.character) | ||
| ); | ||
|
|
||
| // Collapsing multiple leading empty lines to one is a Quarto-level | ||
| // formatting operation: it fires even when no language formatter is active, | ||
| // so we build this edit before the early-returns below. | ||
| // Use the document's line ending to avoid introducing mixed EOL in CRLF files. | ||
| const eol = doc.eol === EndOfLine.CRLF ? "\r\n" : "\n"; | ||
| const normalizeEdit: TextEdit | undefined = leadingEmptyLines > 1 | ||
| ? new TextEdit( | ||
| new Range( | ||
| new Position(block.range.start.line + 1 + optionLines, 0), | ||
| new Position(block.range.start.line + 1 + optionLines + leadingEmptyLines, 0) | ||
| ), | ||
| eol | ||
| ) | ||
| : undefined; | ||
|
|
||
| // Nothing to format if the block is entirely option directives (or only | ||
| // trailing whitespace after them, which `lines()` may produce from a | ||
| // final newline in `token.data`). | ||
| // final newline in `token.data`). Still apply normalizeEdit if present. | ||
| if (codeLines.every(l => l.trim() === "")) { | ||
| return undefined; | ||
| if (normalizeEdit && !blockRange.contains(normalizeEdit.range)) { | ||
| return undefined; | ||
|
Comment on lines
353
to
+355
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non blocking note: In terms of code style / ease of maintenance, it might be nice if we could structure this function to not have early exists with hard to remember corner cases like the So, like, in this case what you might consider doing is assigning In my head, the benefit of this is that the only time and otherwise you don't have to think about it, which would be quite nice |
||
| } | ||
|
Comment on lines
+354
to
+356
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how this could happen, it looks like the way you defined
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (If you remove it here, also remove it again below) (If you don't remove it, maybe ensure we have a test that shows why it is required) |
||
| return normalizeEdit ? [normalizeEdit] : undefined; | ||
| } | ||
|
|
||
| const vdoc = virtualDocForCode(codeLines, language); | ||
|
|
||
| const edits = await executeFormatDocumentProvider( | ||
|
|
@@ -330,19 +368,18 @@ async function formatBlock( | |
| if (!edits || edits.length === 0) { | ||
| // Either no formatter picked us up, or there were no edits required. | ||
| // We can't determine the difference though! | ||
| return undefined; | ||
| if (normalizeEdit && !blockRange.contains(normalizeEdit.range)) { | ||
| return undefined; | ||
| } | ||
| return normalizeEdit ? [normalizeEdit] : undefined; | ||
| } | ||
|
|
||
| // Because we format with the block code copied in an empty virtual | ||
| // document, we need to adjust the ranges to match the edits to the block | ||
| // cell in the original file. The `+ 1` skips the opening fence line and | ||
| // cell in the original file. The `+ 1` skips the opening fence line, | ||
| // `+ optionLines` skips the leading option directives we hid from the | ||
| // formatter. | ||
| const lineOffset = block.range.start.line + 1 + optionLines; | ||
| const blockRange = new Range( | ||
| new Position(block.range.start.line, block.range.start.character), | ||
| new Position(block.range.end.line, block.range.end.character) | ||
| ); | ||
| // formatter, and `+ leadingEmptyLines` skips the leading empty lines. | ||
| const lineOffset = block.range.start.line + 1 + optionLines + leadingEmptyLines; | ||
| const adjustedEdits = edits.map(edit => { | ||
| const range = new Range( | ||
| new Position(edit.range.start.line + lineOffset, edit.range.start.character), | ||
|
|
@@ -351,6 +388,12 @@ async function formatBlock( | |
| return new TextEdit(range, edit.newText); | ||
| }); | ||
|
|
||
| // Include normalizeEdit in the guard so it is validated along with formatter | ||
| // edits — all edits must be in range or none are applied. | ||
|
Comment on lines
+391
to
+392
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again, it seems like you built the normalize edit from the original range itself, so this feels circuitous to me, but maybe im missing something. |
||
| if (normalizeEdit) { | ||
| adjustedEdits.push(normalizeEdit); | ||
| } | ||
|
|
||
| // Bail if any edit is out of range. We used to filter these edits out but | ||
| // this could bork the cell. Return `[]` to indicate that we tried. | ||
| if (adjustedEdits.some(edit => !blockRange.contains(edit.range))) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| --- | ||
| title: Leading Empty Lines in Python Code Cells | ||
| format: html | ||
| --- | ||
|
|
||
| ```{python} | ||
| #| label: one-empty-line | ||
|
|
||
| x=1;y=2 | ||
| ``` | ||
|
|
||
| ```{python} | ||
| #| label: two-empty-lines | ||
|
|
||
|
|
||
| x=3;y=4 | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| --- | ||
| title: Leading Empty Lines in R Code Cells | ||
| format: html | ||
| --- | ||
|
|
||
| ```{r} | ||
| #| label: one-empty-line | ||
|
|
||
| x<-1 | ||
| ``` | ||
|
|
||
| ```{r} | ||
| #| label: two-empty-lines | ||
|
|
||
|
|
||
| x<-2 | ||
| ``` | ||
|
|
||
| ```{r} | ||
| #| label: no-empty-lines | ||
| x<-3 | ||
| ``` |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -163,6 +163,19 @@ function mangleHashPipeLines(sourceText: string): string { | |||
| ); | ||||
| } | ||||
|
|
||||
| function rAssignmentFormatter(sourceText: string): string { | ||||
| return sourceText.replace(/(\w)<-(\w)/g, "$1 <- $2"); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Hostile formatter that mangles any leading newline in the virtual document. | ||||
| * If leading empty lines leak into the virtual document, this formatter will | ||||
| * corrupt the cell with a detectable marker. | ||||
| */ | ||||
| function leadingNewlineMangler(sourceText: string): string { | ||||
| return sourceText.replace(/^\n/, "LEAKED_EMPTY_LINE\n"); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Hostile R formatter that rewrites `#|` directives and normalises the | ||||
| * assignment arrow. | ||||
|
|
@@ -512,6 +525,183 @@ suite("Code Block Formatting", function () { | |||
| } | ||||
| }); | ||||
|
|
||||
| test("single leading empty line in R cell is preserved after formatting", async function () { | ||||
| const formattedResult = await testFormatter( | ||||
| "format-r-leading-empty-lines.qmd", | ||||
| [8, 0], | ||||
| rAssignmentFormatter, | ||||
| "r" | ||||
| ); | ||||
|
|
||||
| assert.ok( | ||||
| formattedResult.includes("#| label: one-empty-line"), | ||||
| "Option directive should be preserved" | ||||
| ); | ||||
| assert.ok( | ||||
| formattedResult.includes("x <- 1"), | ||||
| "Code should be reformatted" | ||||
| ); | ||||
| assert.ok( | ||||
| /one-empty-line\n\n/.test(formattedResult), | ||||
| "Single leading empty line should be preserved" | ||||
| ); | ||||
| assert.ok( | ||||
| !/one-empty-line\n\n\n/.test(formattedResult), | ||||
| "No extra empty line should be introduced" | ||||
| ); | ||||
| }); | ||||
|
Comment on lines
+528
to
+552
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another non blocking note: I think these kinds of tests are much easier to skim if they are structured as snapshot tests, kind of like https://github.com/quarto-dev/quarto/blob/5f420f8788de39ea4b37f33dd1cba0a3268db5a8/apps/vscode/src/test/vdoc.test.ts The idea being that for each test you have some on-disk
That way its fairly easy to just look at Each test could be structured as a folder, like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also see
|
||||
|
|
||||
| test("multiple leading empty lines in R cell are collapsed to one after formatting", async function () { | ||||
| const formattedResult = await testFormatter( | ||||
| "format-r-leading-empty-lines.qmd", | ||||
| [13, 0], | ||||
| rAssignmentFormatter, | ||||
| "r" | ||||
| ); | ||||
|
|
||||
| assert.ok( | ||||
| formattedResult.includes("#| label: two-empty-lines"), | ||||
| "Option directive should be preserved" | ||||
| ); | ||||
| assert.ok( | ||||
| formattedResult.includes("x <- 2"), | ||||
| "Code should be reformatted" | ||||
| ); | ||||
| assert.ok( | ||||
| /two-empty-lines\n\n/.test(formattedResult), | ||||
| "Exactly one leading empty line should remain" | ||||
| ); | ||||
| assert.ok( | ||||
| !/two-empty-lines\n\n\n/.test(formattedResult), | ||||
| "Second leading empty line should be collapsed" | ||||
| ); | ||||
| }); | ||||
|
|
||||
| test("no leading empty lines in R cell — unaffected by the normalisation", async function () { | ||||
| const formattedResult = await testFormatter( | ||||
| "format-r-leading-empty-lines.qmd", | ||||
| [20, 0], | ||||
| rAssignmentFormatter, | ||||
| "r" | ||||
| ); | ||||
|
|
||||
| assert.ok( | ||||
| formattedResult.includes("#| label: no-empty-lines"), | ||||
| "Option directive should be preserved" | ||||
| ); | ||||
| assert.ok( | ||||
| formattedResult.includes("x <- 3"), | ||||
| "Code should be reformatted" | ||||
| ); | ||||
| assert.ok( | ||||
| /no-empty-lines\nx <- 3/.test(formattedResult), | ||||
| "No empty line should be introduced" | ||||
| ); | ||||
| }); | ||||
|
|
||||
| test("leading empty lines in Python cell are preserved after formatting", async function () { | ||||
| const formattedResult = await testFormatter( | ||||
| "format-python-leading-empty-lines.qmd", | ||||
| [8, 0], | ||||
| spaceAssignments | ||||
| ); | ||||
|
|
||||
| assert.ok( | ||||
| formattedResult.includes("#| label: one-empty-line"), | ||||
| "Option directive should be preserved" | ||||
| ); | ||||
| assert.ok( | ||||
| formattedResult.includes("x = 1"), | ||||
| "Code should be reformatted" | ||||
| ); | ||||
| assert.ok( | ||||
| /one-empty-line\n\n/.test(formattedResult), | ||||
| "Single leading empty line should be preserved" | ||||
| ); | ||||
| assert.ok( | ||||
| !/one-empty-line\n\n\n/.test(formattedResult), | ||||
| "No extra empty line should be introduced" | ||||
| ); | ||||
| }); | ||||
|
|
||||
| test("multiple leading empty lines in Python cell are collapsed to one after formatting", async function () { | ||||
| const formattedResult = await testFormatter( | ||||
| "format-python-leading-empty-lines.qmd", | ||||
| [13, 0], | ||||
| spaceAssignments | ||||
| ); | ||||
|
|
||||
| assert.ok( | ||||
| formattedResult.includes("#| label: two-empty-lines"), | ||||
| "Option directive should be preserved" | ||||
| ); | ||||
| assert.ok( | ||||
| formattedResult.includes("x = 3"), | ||||
| "Code should be reformatted" | ||||
| ); | ||||
| assert.ok( | ||||
| /two-empty-lines\n\n/.test(formattedResult), | ||||
| "Exactly one leading empty line should remain" | ||||
| ); | ||||
| assert.ok( | ||||
| !/two-empty-lines\n\n\n/.test(formattedResult), | ||||
| "Second leading empty line should be collapsed" | ||||
| ); | ||||
| }); | ||||
|
|
||||
| test("leading empty lines are hidden from the formatter", async function () { | ||||
| // Target the two-empty-lines cell: without stripping, the virtual doc | ||||
| // would start with "\n\nx<-2" and the mangler would inject LEAKED_EMPTY_LINE. | ||||
| // With stripping, the virtual doc starts with "x<-2" and the mangler is silent. | ||||
| const formattedResult = await testFormatter( | ||||
| "format-r-leading-empty-lines.qmd", | ||||
| [13, 0], | ||||
| leadingNewlineMangler, | ||||
| "r" | ||||
| ); | ||||
|
|
||||
| assert.ok( | ||||
| !formattedResult.includes("LEAKED_EMPTY_LINE"), | ||||
| "Leading empty lines must not be visible to the formatter" | ||||
| ); | ||||
| assert.ok( | ||||
| /two-empty-lines\n\n/.test(formattedResult), | ||||
| "Exactly one leading empty line should remain in the cell after formatting" | ||||
| ); | ||||
| assert.ok( | ||||
| !/two-empty-lines\n\n\n/.test(formattedResult), | ||||
| "Second leading empty line should be collapsed" | ||||
| ); | ||||
| }); | ||||
|
|
||||
| test("multiple leading empty lines are collapsed without a language formatter", async function () { | ||||
| const { doc } = await openAndShowExamplesTextDocument( | ||||
| "format-r-leading-empty-lines.qmd" | ||||
| ); | ||||
|
|
||||
| try { | ||||
| // No formatter is registered for "r" — only the Quarto-level | ||||
| // normalisation edit should fire. | ||||
| setCursorPosition(13, 0); | ||||
| await wait(450); | ||||
| await vscode.commands.executeCommand("quarto.formatCell"); | ||||
| await wait(450); | ||||
|
|
||||
| const result = doc.getText(); | ||||
|
|
||||
| assert.ok( | ||||
| /two-empty-lines\n\n/.test(result), | ||||
| "Exactly one leading empty line should remain" | ||||
| ); | ||||
| assert.ok( | ||||
| !/two-empty-lines\n\n\n/.test(result), | ||||
| "Second leading empty line should be collapsed even without a formatter" | ||||
| ); | ||||
| } finally { | ||||
| await vscode.commands.executeCommand("workbench.action.closeActiveEditor"); | ||||
| } | ||||
| }); | ||||
|
|
||||
| test("formatter returning multiple discrete edits is applied correctly", async function () { | ||||
| const { doc } = await openAndShowExamplesTextDocument( | ||||
| "format-python-multiple-options.qmd" | ||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this different from
block.range, which already exists?