Skip to content
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.

Sign up for GitHub

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

Fix: Indent multiline fixes (fixes #120) #124

Merged
merged 5 commits into from
Oct 22, 2019
Merged

Fix: Indent multiline fixes (fixes #120) #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5502a18
Chore: Add failing test case for indent of multiline fixes
lydell Sep 7, 2019
f308f99
Fix: Indent multiline fixes (fixes #120)
btmills Oct 8, 2019
0e14506
Add test for underindented multiline autofix
btmills Oct 8, 2019
74c1473
Add test for nested blockquote multiline autofix
btmills Oct 8, 2019
80edd59
Add comment explaining underintented line shift
btmills Oct 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions lib/processor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,33 @@ function getComment(html) {
return comment;
}

const leadingWhitespaceRegex = /^\s*/;
// Before a code block, blockquote characters (`>`) are also considered
// "whitespace".
const leadingWhitespaceRegex = /^[>\s]*/;

/**
* Gets the offset for the first column of the node's first line in the
* original source text.
* @param {ASTNode} node A Markdown code block AST node.
* @returns {number} The offset for the first column of the node's first line.
*/
function getBeginningOfLineOffset(node) {
return node.position.start.offset - node.position.start.column + 1;
}

/**
* Gets the leading text, typically whitespace with possible blockquote chars,
* used to indent a code block.
* @param {string} text The text of the file.
* @param {ASTNode} node A Markdown code block AST node.
* @returns {string} The text from the start of the first line to the opening
* fence of the code block.
*/
function getIndentText(text, node) {
return leadingWhitespaceRegex.exec(
text.slice(getBeginningOfLineOffset(node))
)[0];
}

/**
* When applying fixes, the postprocess step needs to know how to map fix ranges
Expand Down Expand Up @@ -110,7 +136,7 @@ function getBlockRangeMap(text, node, comments) {
* additional indenting, the opening fence's first backtick may be up to
* three whitespace characters after the start offset.
*/
const startOffset = node.position.start.offset;
const startOffset = getBeginningOfLineOffset(node);

/*
* Extract the Markdown source to determine the leading whitespace for each
Expand All @@ -125,8 +151,7 @@ function getBlockRangeMap(text, node, comments) {
* backtick's column is the AST node's starting column plus any additional
* indentation.
*/
const baseIndent = node.position.start.column - 1 +
leadingWhitespaceRegex.exec(lines[0])[0].length;
const baseIndent = getIndentText(text, node).length;

/*
* Track the length of any inserted configuration comments at the beginning
Expand Down Expand Up @@ -182,6 +207,7 @@ function getBlockRangeMap(text, node, comments) {
mdOffset += line.length + 1;
jsOffset += line.length - trimLength + 1;
}

return rangeMap;
}

Expand Down Expand Up @@ -219,6 +245,7 @@ function preprocess(text) {
}

blocks.push(assign({}, node, {
baseIndentText: getIndentText(text, node),
comments,
rangeMap: getBlockRangeMap(text, node, comments)
}));
Expand Down Expand Up @@ -281,7 +308,7 @@ function adjustBlock(block) {
// Apply the mapping delta for this range.
return range + block.rangeMap[i - 1].md;
}),
text: message.fix.text
text: message.fix.text.replace("\n", `\n${block.baseIndentText}`)
};
}

Expand Down
118 changes: 118 additions & 0 deletions tests/lib/plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,124 @@ describe("plugin", () => {
assert.strictEqual(actual, expected);
});

it("multiline autofix", () => {
const input = [
"This is Markdown.",
"",
" ```js",
" console.log('Hello, \\",
" world!')",
" console.log('Hello, \\",
" world!')",
" ```"
].join("\n");
const expected = [
"This is Markdown.",
"",
" ```js",
" console.log(\"Hello, \\",
" world!\")",
" console.log(\"Hello, \\",
" world!\")",
" ```"
].join("\n");
const report = cli.executeOnText(input, "test.md");
const actual = report.results[0].output;

assert.strictEqual(actual, expected);
});

it("underindented multiline autofix", () => {
const input = [
" ```js",
" console.log('Hello, world!')",
" console.log('Hello, \\",
" world!')",
" console.log('Hello, world!')",
" ```"
].join("\n");

// The Markdown parser doesn't have any concept of a "negative"
// indent left of the opening code fence, so autofixes move
// lines that were previously underindented to the same level
// as the opening code fence.
const expected = [
" ```js",
" console.log(\"Hello, world!\")",
" console.log(\"Hello, \\",
" world!\")",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not 100% sure how leading whitespace works in weird code blocks like this, but didn’t we cause one more space to appear in the JS string here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed 80edd59 that adds comments explaining why this occurs for the benefit of future readers of these tests.

When a line inside a fenced code block is indented less than the opening fence, Markdown treats it as having 0 indent, even though its indent in the source is negative. The parser doesn't provide any information to distinguish lines that begin at the same level as the opening fence from those that are underindented. Whenever one of those lines gets autofixed, it gets shifted to align with the opening fence. Lines that that have positive indentation inside the code fence retain their original indentation because those are representable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this case in https://spec.commonmark.org/dingus/ and the actual and expected code snippets produce the same rendered JS, with the same amount of spaces between "Hello," and "world!".

" console.log(\"Hello, world!\")",
" ```"
].join("\n");
const report = cli.executeOnText(input, "test.md");
const actual = report.results[0].output;

assert.strictEqual(actual, expected);
});

it("multiline autofix in blockquote", () => {
const input = [
"This is Markdown.",
"",
"> ```js",
"> console.log('Hello, \\",
"> world!')",
"> console.log('Hello, \\",
"> world!')",
"> ```"
].join("\n");
const expected = [
"This is Markdown.",
"",
"> ```js",
"> console.log(\"Hello, \\",
"> world!\")",
"> console.log(\"Hello, \\",
"> world!\")",
"> ```"
].join("\n");
const report = cli.executeOnText(input, "test.md");
const actual = report.results[0].output;

assert.strictEqual(actual, expected);
});

it("multiline autofix in nested blockquote", () => {
const input = [
"This is Markdown.",
"",
"> This is a nested blockquote.",
">",
"> > ```js",
"> > console.log('Hello, \\",
"> > world!')",
"> > console.log('Hello, \\",
"> > world!')",
"> > ```"
].join("\n");

// The Markdown parser doesn't have any concept of a "negative"
// indent left of the opening code fence, so autofixes move
// lines that were previously underindented to the same level
// as the opening code fence.
const expected = [
"This is Markdown.",
"",
"> This is a nested blockquote.",
">",
"> > ```js",
"> > console.log(\"Hello, \\",
"> > world!\")",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here? Is this 1–2 too many spaces?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, same cause. I added comments in 80edd59 and explained further in #124 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this case in https://spec.commonmark.org/dingus/ and the actual and expected code snippets produce the same rendered JS, with the same amount of spaces between "Hello," and "world!".

"> > console.log(\"Hello, \\",
"> > world!\")",
"> > ```"
].join("\n");
const report = cli.executeOnText(input, "test.md");
const actual = report.results[0].output;

assert.strictEqual(actual, expected);
});

it("by one space with comments", () => {
const input = [
"This is Markdown.",
Expand Down