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

Jump to bottom

Fix embedded templates handling in block-indentation rule #2838

Merged
merged 2 commits into from Mar 15, 2023
Merged

Fix embedded templates handling in block-indentation rule #2838

merged 2 commits into from Mar 15, 2023

Conversation

robinborst95
Copy link
Contributor

This partially fixes the issue described in #2826. The example in that issue is a combination of block-indentation and attribute-indentation I think, and this PR only fixes the block-indentation, which I guess is more important to fix than attribute indentation. I've added some examples to the tests that work now (including autofix), but I'm happy to get feedback on other scenarios that need to be tested as well. An example of what this PR fixes:

import { hbs } from 'ember-cli-htmlbars';

test('it renders', async (assert) => {
  await render(hbs`
    <div class="parent">
      <div class="child"></div>
    </div>
  `);
);

Before this PR, the linter would only be happy if it were formatted like:

import { hbs } from 'ember-cli-htmlbars';

test('it renders', async (assert) => {
  await render(hbs`
<div class="parent">
  <div class="child"></div>
</div>
  `);
);

which is not very pretty and intuitive.

@robinborst95 robinborst95 force-pushed the fix/block-indentation-embedded branch 2 times, most recently from 7f13c58 to 9e0d61f Compare March 8, 2023 12:56
robinborst95
robinborst95 commented Mar 8, 2023
View reviewed changes
lib/extract-templates.js
@@ -2,7 +2,7 @@ import { parseTemplates } from 'ember-template-imports/lib/parse-templates.js';
import * as util from 'ember-template-imports/src/util.js';

export const SUPPORTED_EXTENSIONS = ['.js', '.ts', '.gjs', '.gts'];
const LOCATION_START = Object.freeze({ line: 0, column: 0, start: 0, end: 0 });
const LOCATION_START = Object.freeze({ line: 0, column: 0, start: 0, end: 0, columnOffset: 0 });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way I approached it, is that the location of the embedded template is offset by the indentation of the line the template starts at. So, something like

test('it renders', async (assert) => {
  await render(hbs`

has an offset of 2, because await has an indentation of 2 spaces. Based on this info, each rule (block-indentation in this case, but attribute-indentation can probably work the same) can adjust the expected indentation if needed.

lib/rules/block-indentation.js
function removeWhitespaceEnd(str) {
return str.replace(/ +$/, '');
function addWhitespaceEnd(str, count) {
return `${str}${' '.repeat(count)}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See line 326. The direct child indentation used to only remove whitespaces, but in embedded templates adding whitespaces is now also necessary sometimes.

lib/rules/block-indentation.js
@@ -306,13 +310,20 @@ export default class BlockIndentation extends Rule {
const hasLeadingContent =
!isDirectChildOfTemplate || this.hasLeadingContent(node, this.template.body);
const startColumn = node.loc.start.column;
const expectedStartColumn =
this.columnOffset === 0 || node.loc.start.line === 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 'root' node starting at line 1 is meant for cases like:

test('it renders', async (assert) => {
  await render(hbs`<div class="parent">
    <div class="child"></div>
  </div>`);
);

lib/rules/block-indentation.js
@@ -347,8 +358,12 @@ export default class BlockIndentation extends Rule {

let controlCharCount = this.endingControlCharCount(node);
let correctedEndColumn = endColumn - displayName.length - controlCharCount;
let expectedEndColumn =
this.columnOffset === 0 || node.loc.end.line !== this.template.loc.end.line
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to the start of a block:

test('it renders', async (assert) => {
  await render(hbs`<div class="parent">
    <div class="child"></div>
  </div>`);
);

When the node it at the end of the template is and the template is embedded, then the start column should be the columnOffset

lib/rules/block-indentation.js
@@ -477,7 +494,7 @@ export default class BlockIndentation extends Rule {

fixedNode = this.fixLine(
lines,
childStartLine - 1,
childStartLine - (path ? 1 : fixedNode.loc.start.line),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually seemed to be an existing bug. When I ran the autofixer with master on our own embedded templates, then I got TypeError: Cannot read property 'startsWith' of undefined because the childStartLine was wrong

test/unit/rules/block-indentation-test.js
},

verifyResults(results) {
expect(results).toMatchInlineSnapshot(`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Creating these snapshots was not trivial to do, is there a command for this?

Copy link
Member

Choose a reason for hiding this comment

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

jest --updateSnapshot ?

@robinborst95 robinborst95 marked this pull request as ready for review March 8, 2023 13:03
@robinborst95 robinborst95 mentioned this pull request Mar 8, 2023
Closed
@robinborst95 robinborst95 force-pushed the fix/block-indentation-embedded branch from 9e0d61f to 23e6fa3 Compare March 8, 2023 22:05
@robinborst95 robinborst95 mentioned this pull request Mar 8, 2023
Merged
@bmish bmish added the bug label Mar 14, 2023
bmish
bmish approved these changes Mar 15, 2023
View reviewed changes
Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

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

Seems reasonable, thanks! What part of #2826 is still not fixed? CC: @tylerbecks

@bmish bmish changed the title Fix block-indentation rule for embedded templates Fix embedded templates handling in block-indentation rule Mar 15, 2023
@bmish bmish merged commit 184d025 into ember-template-lint:master Mar 15, 2023
@robinborst95
Copy link
Contributor Author

Thanks for merging! The part of #2826 that is not fixed is actually the first example mentioned there. I think it's the attribute-indentation rule that does this, and that's not fixed with this PR of course. However, when the attribute-indentation is not enabled or not violated, the same issue with indentation would occur, so that's why I described it as partially fixed.

@robinborst95 robinborst95 deleted the fix/block-indentation-embedded branch March 15, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmish bmish bmish approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@robinborst95 @bmish