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.

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 false negatives with block-indentation rule #2318

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 29 additions & 23 deletions lib/rules/block-indentation.js
Expand Up @@ -192,18 +192,14 @@ module.exports = class BlockIndentation extends Rule {
this.seen = new Set();

return {
Template(node) {
if (this.mode === 'fix') {
if (
node.body[0] &&
node.body[0].type === 'TextNode' &&
!removeWhitespaceEnd(node.body[0].chars)
) {
// We need to remove the text node in this case
node.body.shift();
}
}
return node;
Template: {
enter(node) {
this.template = node;
return node;
},
exit() {
this.template = null;
},
},

BlockStatement(node) {
Expand Down Expand Up @@ -281,7 +277,7 @@ module.exports = class BlockIndentation extends Rule {
}

// Recreate the node only if the source has changed, otherwise this would mean useless computation and possible mistake
if (sourceBeforeFix !== this.sourceEdited) {
if (sourceBeforeFix !== this.sourceEdited && this.sourceEdited) {
fixedNode = this.fixLine(
this.sourceEdited.split('\n'),
0,
Expand All @@ -302,20 +298,30 @@ module.exports = class BlockIndentation extends Rule {
return node;
}

const indexOfNodeInTemplate = this.template.body.indexOf(node);
const isDirectChildOfTemplate = indexOfNodeInTemplate !== -1;
let startColumn = node.loc.start.column;
let startLine = node.loc.start.line;
const shouldValidate = startLine === 1 || isDirectChildOfTemplate;

if (startLine === 1 && startColumn !== 0) {
if (shouldValidate && startColumn !== 0) {
if (this.mode === 'fix') {
// If we are in a child (path set) we want to edit directly the source and not the source of the node
const elementSource = path
? this.sourceEdited || this.source.join('')
: sourceForLoc(this.sourceEdited || this.source, {
end: node.loc.end,
start: { line: node.loc.start.line, column: 0 },
});
const fixedNode = this.fixLine(elementSource.split('\n'), 0, startColumn, 0, path);
return fixedNode;
if (isDirectChildOfTemplate) {
const previousNode = this.template.body[indexOfNodeInTemplate - 1];
if (previousNode.type === 'TextNode') {
previousNode.chars = removeWhitespaceEnd(previousNode.chars);
}
} else {
// If we are in a child (path set) we want to edit directly the source and not the source of the node
const elementSource = path
? this.sourceEdited || this.source.join('')
: sourceForLoc(this.sourceEdited || this.source, {
end: node.loc.end,
start: { line: node.loc.start.line, column: 0 },
});
const fixedNode = this.fixLine(elementSource.split('\n'), 0, startColumn, 0, path);
return fixedNode;
}
} else {
let isElementNode = node && node.type === 'ElementNode';
let displayName = isElementNode ? node.tag : node.path.original;
Expand Down
43 changes: 40 additions & 3 deletions test/unit/rules/block-indentation-test.js
Expand Up @@ -26,7 +26,7 @@ generateRuleTests({
' <iframe src="some_url"></iframe>',
'{{/if}}',
].join('\n'),
'\n {{#each cats as |dog|}}\n {{/each}}',
'\n{{#each cats as |dog|}}\n{{/each}}',
'<div><p>Stuff</p></div>',
'<div>\n <p>Stuff Here</p>\n</div>',
'{{#if isMorning}}\n' +
Expand Down Expand Up @@ -227,17 +227,30 @@ generateRuleTests({
'{{/if}}',
].join('\n'),
},
],
].filter((value, index) => index >= 0),
VincentMolinie marked this conversation as resolved.
Show resolved Hide resolved

bad: [
{
// start and end must be the same indentation
template: '\n {{#each cats as |dog|}}\n {{/each}}',
fixedTemplate: '\n {{#each cats as |dog|}}\n {{/each}}',
fixedTemplate: '\n{{#each cats as |dog|}}\n{{/each}}',

verifyResults(results) {
expect(results).toMatchInlineSnapshot(`
Array [
Object {
"column": 2,
"endColumn": 17,
"endLine": 3,
"filePath": "layout.hbs",
"isFixable": true,
"line": 2,
"message": "Incorrect indentation for \`{{#each}}\` beginning at L2:C2. Expected \`{{#each}}\` to be at an indentation of 0, but was found at 2.",
"rule": "block-indentation",
"severity": 2,
"source": "{{#each cats as |dog|}}
{{/each}}",
},
Object {
"column": 17,
"endColumn": 17,
Expand Down Expand Up @@ -1182,5 +1195,29 @@ generateRuleTests({
`);
},
},
{
template: '<div>\n</div>\n <span>\n </span>',
fixedTemplate: '<div>\n</div>\n<span>\n</span>',

verifyResults(results) {
expect(results).toMatchInlineSnapshot(`
Array [
Object {
"column": 2,
"endColumn": 9,
"endLine": 4,
"filePath": "layout.hbs",
"isFixable": true,
"line": 3,
"message": "Incorrect indentation for \`<span>\` beginning at L3:C2. Expected \`<span>\` to be at an indentation of 0, but was found at 2.",
"rule": "block-indentation",
"severity": 2,
"source": "<span>
</span>",
},
]
`);
},
},
],
});