Support prettier-ignore comments inside pug templates

[lehni]

Use //- prettier-ignore comments to prevent automatic formatting of the following line or block:

div
  .wrapper-1(attributes1="foo", :attribute2="bar", attribute3="something too long to keep on one line")
  //- prettier-ignore
  .wrapper-2(attributes1="foo", :attribute2="bar", attribute3="something too long to keep on one line")
  .wrapper-3(attributes1="foo", :attribute2="bar", attribute3="something too long to keep on one line")
  //- prettier-ignore
  div
    .wrapper-4(attributes1="foo", :attribute2="bar", attribute3="something too long to keep on one line")
.wrapper-5(attributes1="foo", :attribute2="bar", attribute3="something too long to keep on one line")

->

div
  .wrapper-1(
    attributes1="foo",
    :attribute2="bar",
    attribute3="something too long to keep on one line"
  )
  //- prettier-ignore
  .wrapper-2(attributes1="foo", :attribute2="bar", attribute3="something too long to keep on one line")
  .wrapper-3(
    attributes1="foo",
    :attribute2="bar",
    attribute3="something too long to keep on one line"
  )
  //- prettier-ignore
  div
    .wrapper-4(attributes1="foo", :attribute2="bar", attribute3="something too long to keep on one line")
.wrapper-5(
  attributes1="foo",
  :attribute2="bar",
  attribute3="something too long to keep on one line"
)

[lehni]

@Shinigami92 I tried to request a review but for some reason, the button doesn't show up on this PR

[lehni]

It's up for debate if it only ever should ignore the next line, or all nested elements as well (e.g. the 2nd example above, the div with nested .wrapper-4).

Personally I find it more pug-like if it affects the nested things as well. That's also how commenting things out works in pug.

Happy to discuss!

[Shinigami92]

@Shinigami92 I tried to request a review but for some reason, the button doesn't show up on this PR

Ok I need to investigate why my community cannot request these. Maybe it's a GitHub Teams feature to do so 🤔
Or I need to relax permissions somewhere

But thx for telling me 🙂

[lehni]

@Shinigami92 this used to work, I think? Maybe it was something you changed recently?

[Shinigami92]

It's up for debate if it only ever should ignore the next line, or all nested elements as well (e.g. the 2nd example above, the div with nested .wrapper-4).

Personally I find it more pug-like if it affects the nested things as well. That's also how commenting things out works in pug.

Happy to discuss!

Very interesting idea 🤔
But is it hard to implement?

I mean I need to look in your current working code 😅

[lehni]

The current implementation does that already! 🤣

[Shinigami92]

Fucking hell, can I come across the globe and just hug you?!
Damn it, this is rocket science

I really start to think about to give both of you (also @SkyaTura) more privileges in my repo

But, need to lock into it with IDE in depth
Will go home now and work further from there 😃

[Shinigami92]

@lehni and @SkyaTura: If you didn't already noticed ->
I gave both of you Role: Write privileges to my repo 🎉
So now you can delete your forks (when I have merged all current PRs from you), and checkout this origin directly
You both cannot work on master-branch but you can freely create your own branches in this repo

when creating branches directly in this repo, it's easier for me to check them out and review them with vscode 🙂

Also I think, you can add and change some labels and assignees etc in issues and PRs 🤔

[lehni]

I'm away from the computer now. Will write more later. Thanks!

[Shinigami92]

So if I understood everything correct, the following wouldn't make sense, and therefore is correctly commented?

div
  //- prettier-ignore | (1)
  div
    //- prettier-ignore | <-- already ignored be the above one (1)?!
    div
  div
    div
    //- prettier-ignore | (2)
    div
      //- prettier-ignore | <-- already ignored be the above one (2)?!
      div
  div

---

Should we allow a reason?

div
  //- prettier-ignore: I want this this way!
  div

[lehni]

So if I understood everything correct, the following wouldn't make sense, and therefore is correctly commented?

div
  //- prettier-ignore | (1)
  div
    //- prettier-ignore | <-- already ignored be the above one (1)?!
    div
  div
    div
    //- prettier-ignore | (2)
    div
      //- prettier-ignore | <-- already ignored be the above one (2)?!
      div
  div

Yes, exactly! It's kind of similar to what happens when you comment out a block of pug code:

// everything indented underneath will be commented out:
  div
    div
  div
    div
    div
      div
  div

Should we allow a reason?

div
  //- prettier-ignore: I want this this way!
  div

Yes, we could simply match /^\s*prettier-ignore\b/ and whatever follows is up to you.

[Shinigami92]

I really like that feature, but we should check this against a code base with multiple //- prettier-ignore on some places and watch the performance
Can you test this somewhere?

[lehni]

@Shinigami92 I am happy to add more tests for various scenarios of nesting, was thinking the same!

But I don't think we need a performance test, and that would be hard to even test reliably (something else could slow down the test runner as it is running these tests and times them. you'd have to run them over and over again).

Reasons for why I don't think a test for speed is necessary:

When the build()loop encounters a prettier-ignore comment, it then uses a simple loop in comment() to find the end of the stream of tokens to skip / leave unformatted. That loop uses a very simple logic that is faster than the normal processing of these tokens would be. The total amount of tokens treated in one loop or the other doesn't change. Once the code in comment() has found the range of tokens to leave unformatted, it uses the loc property of the first and the last token to simply get the unformatted string from the original pug content. This again isn't expensive, no complexity involved. (see: getUnformattedContentLines()).

Bottom line: pug code that contains prettier-ignore comments will most likely process faster than the same code without those comments, because there is less formatting and processing involved, not more.

[lehni]

I'm away from the computer mostly today, will wrap this up tomorrow probably! Thanks for the reviews @Shinigami92!

[Shinigami92]

@lehni I didn't mean an automated test, I just meant a one time study
So you use yarn link to link this repo against a code base using the feature and then see if the processing of //- prettier-ignore takes much longer as not processing it
And if so, we should write a warning note into readme, to not use this feature heavily, otherwise everything is fine

[lehni]

@Shinigami92 I did a little speed test now, using my improved test in prettier-ignore.test.js, with some simple modifications to run format() 100 times instead of once, and measure the time:

const expected: string = readFileSync(resolve(__dirname, 'formatted.pug'), 'utf8');
const code: string = readFileSync(resolve(__dirname, 'unformatted.pug'), 'utf8');
let actual: string = '';
console.time('format')
for (let i = 0; i < 100; i++) {
	actual = format(code, { parser: 'pug' as any, plugins: [plugin] });
}
console.timeEnd('format')

Here are the times:

With handling of //- prettier-ignore comments from this PR: 2685 ms
With the current master branch: 5223 ms

So the changes in this PR actually make the processing run twice as fast in this case, because less formatting needs to be performed on the parts that can remain unformatted. This confirms my expectations formulated above, I think.

[lehni]

I noticed that the above test is a bit tricky because it's slowed down by logging. If I run with the --silent flag, then I don't see the timeEnd() measurement. But this ugly trick allows to run silent and still see the times:

const time = Date.now()
for (let i = 0; i < 100; i++) {
	actual = format(code, { parser: 'pug' as any, plugins: [plugin] });
}
expect(Date.now() - time).toBe(0);

And with this, I get the following times:

master: 1058ms
#125: 677ms

So not quite twice as fast, but 1.56 times faster still.

[Shinigami92]

@lehni Is something still missing after the conflict has been resolved or can I merge it then?

[lehni]

@Shinigami92 No this is all good from my side! I've resolved the conflict also.