Repository: prettier/plugin-pug
Pull request #503 (2024-09-04 19:13:25)

Fix class attributes not counting towards attribute wrapping
### Issue

There are a few issues with the `pugClassNotation: 'attribute'` option that resolve themselves with multiple repeated runs (one run to convert from literals to attributes with broken formatting, another run to fix the formatting) but one issue persists.

Classes are not counted to `numNormalAttributes` when deciding if the attributes should be wrapped as per `pugWrapAttributesThreshold`. This causes lines not to wrap as expected if they have class attributes.

Input with `pugWrapAttributesThreshold: 1` and `pugClassNotation: 'attribute'`:

```pug
div(class="class" attribute="value")
```

Output:

```pug
// Still on one line (bad)
div(class="class" attribute="value")
```

Expected output:

```pug
div(
  class="class",
  attribute="value"
)
```

### Fix

I added an extra check to the code path for classes that manually adds +1 to `numNormalAttributes` if classes were defined as attributes and they are not explicitly forced to be converted to literals.

### Test

I added two new tests (better naming welcome) demonstrating correct wrapping when both `pugClassNotation` and `pugWrapAttributesThreshold` have been set. I left the tests simple as more complex setups like mixed literal + attribute classes will hit formatting bugs that should not be in the scope of this PR.

Shinigami92: please update the PR, but I still need to find time to analyze this PR/fix
But going to company-work now 👋 
MrHaila: You are correct that ID's have the same issue. I added a test and expanded the same fix to cover ID attributes.

`pugIdNotation` does not have an `attribute` option, so the only affected case is `as-is`. It would be lovely for it to have the `attribute` as I'd probably prefer that for consistency, but that's another matter.

As an aside, what would be a kosher way to add tests that will currently fail because of a bug? Use the `.fail` to silently accept the failure and comment as a known issue? I could document a few things while I remember them, but they are a bit out of my depth to PR for 🙂

An example is that the attribute wrapping classes will fail with `.class(attribute="value")` because the wrapping logic runs before `.class` is converted to `class="class"`, making the outputted line stay on one row.
Shinigami92: > You are correct that ID's have the same issue. I added a test and expanded the same fix to cover ID attributes.
> 
> `pugIdNotation` does not have an `attribute` option, so the only affected case is `as-is`. It would be lovely for it to have the `attribute` as I'd probably prefer that for consistency, but that's another matter.
>
> As an aside, what would be a kosher way to add tests that will currently fail because of a bug? Use the `.fail` to silently accept the failure and comment as a known issue? I could document a few things while I remember them, but they are a bit out of my depth to PR for 🙂

It's a pleasure to work with you because you think ahead 😸 

I need to have a look into this `.fail` but gut feeling tells me that something with this feels wrong 🤔  

> An example is that the attribute wrapping classes will fail with `.class(attribute="value")` because the wrapping logic runs before `.class` is converted to `class="class"`, making the outputted line stay on one row.

However for the third options, I welcome you in #167 🐱 

- #167
Shinigami92: I will merge this now and then do some dependency updates
Tell me if you want something additional in v3.1.0 or I'm good to go to release

Patch:
diff --git a/src/printer.ts b/src/printer.ts
index 9de56f2..2c27732 100644
--- a/src/printer.ts
+++ b/src/printer.ts
@@ -829,6 +829,15 @@ export class PugPrinter {
         switch (tempToken.name) {
           case 'class':
           case 'id': {
+            // If classes or IDs are defined as attributes and not converted to literals, count them toward attribute wrapping.
+            if (
+              (tempToken.name === 'class' &&
+                this.options.pugClassNotation !== 'literal') ||
+              (tempToken.name === 'id' &&
+                this.options.pugIdNotation !== 'literal')
+            ) {
+              numNormalAttributes++;
+            }
             hasLiteralAttributes = true;
             const val: string = tempToken.val.toString();
             if (isQuoted(val)) {


Test patch:
diff --git a/tests/options/pugWrapAttributesThreshold/withAsIsClassNotation/formatted.pug b/tests/options/pugWrapAttributesThreshold/withAsIsClassNotation/formatted.pug
new file mode 100644
index 0000000..55d8385
--- /dev/null
+++ b/tests/options/pugWrapAttributesThreshold/withAsIsClassNotation/formatted.pug
@@ -0,0 +1,10 @@
+div(class="class")
+div(
+  class="class",
+  attribute="value"
+)
+.class(attribute="value")
+.class(
+  attribute="value",
+  another-attribute="another value"
+)
diff --git a/tests/options/pugWrapAttributesThreshold/withAsIsClassNotation/unformatted.pug b/tests/options/pugWrapAttributesThreshold/withAsIsClassNotation/unformatted.pug
new file mode 100644
index 0000000..43cd60f
--- /dev/null
+++ b/tests/options/pugWrapAttributesThreshold/withAsIsClassNotation/unformatted.pug
@@ -0,0 +1,4 @@
+div(class="class")
+div(class="class" attribute="value")
+div.class(attribute="value")
+div.class(attribute="value" another-attribute="another value")
diff --git a/tests/options/pugWrapAttributesThreshold/withAsIsClassNotation/with-as-is-class-notation.test.ts b/tests/options/pugWrapAttributesThreshold/withAsIsClassNotation/with-as-is-class-notation.test.ts
new file mode 100644
index 0000000..e9bb6fb
--- /dev/null
+++ b/tests/options/pugWrapAttributesThreshold/withAsIsClassNotation/with-as-is-class-notation.test.ts
@@ -0,0 +1,17 @@
+import { compareFiles } from 'tests/common';
+import { describe, expect, it } from 'vitest';
+
+describe('Options', () => {
+  describe('pugWrapAttributesThreshold', () => {
+    it('should count attribute classes toward wrapping if pugClassNotation === as-is', async () => {
+      const { actual, expected } = await compareFiles(import.meta.url, {
+        formatOptions: {
+          pugWrapAttributesThreshold: 1,
+          pugClassNotation: 'as-is',
+        },
+      });
+
+      expect(actual).toBe(expected);
+    });
+  });
+});
diff --git a/tests/options/pugWrapAttributesThreshold/withAsIsIdNotation/formatted.pug b/tests/options/pugWrapAttributesThreshold/withAsIsIdNotation/formatted.pug
new file mode 100644
index 0000000..aabf735
--- /dev/null
+++ b/tests/options/pugWrapAttributesThreshold/withAsIsIdNotation/formatted.pug
@@ -0,0 +1,10 @@
+div(id="foo")
+div(
+  id="foo",
+  attribute="value"
+)
+#foo(attribute="value")
+#foo(
+  attribute="value",
+  another-attribute="another value"
+)
diff --git a/tests/options/pugWrapAttributesThreshold/withAsIsIdNotation/unformatted.pug b/tests/options/pugWrapAttributesThreshold/withAsIsIdNotation/unformatted.pug
new file mode 100644
index 0000000..554f04f
--- /dev/null
+++ b/tests/options/pugWrapAttributesThreshold/withAsIsIdNotation/unformatted.pug
@@ -0,0 +1,4 @@
+div(id="foo")
+div(id="foo" attribute="value")
+div#foo(attribute="value")
+div#foo(attribute="value" another-attribute="another value")
diff --git a/tests/options/pugWrapAttributesThreshold/withAsIsIdNotation/with-as-is-id-notation.test.ts b/tests/options/pugWrapAttributesThreshold/withAsIsIdNotation/with-as-is-id-notation.test.ts
new file mode 100644
index 0000000..c51855e
--- /dev/null
+++ b/tests/options/pugWrapAttributesThreshold/withAsIsIdNotation/with-as-is-id-notation.test.ts
@@ -0,0 +1,17 @@
+import { compareFiles } from 'tests/common';
+import { describe, expect, it } from 'vitest';
+
+describe('Options', () => {
+  describe('pugWrapAttributesThreshold', () => {
+    it('should count attribute IDs toward wrapping if pugIdNotation === as-is', async () => {
+      const { actual, expected } = await compareFiles(import.meta.url, {
+        formatOptions: {
+          pugWrapAttributesThreshold: 1,
+          pugIdNotation: 'as-is',
+        },
+      });
+
+      expect(actual).toBe(expected);
+    });
+  });
+});
diff --git a/tests/options/pugWrapAttributesThreshold/withAttributeClassNotation/formatted.pug b/tests/options/pugWrapAttributesThreshold/withAttributeClassNotation/formatted.pug
new file mode 100644
index 0000000..052ac56
--- /dev/null
+++ b/tests/options/pugWrapAttributesThreshold/withAttributeClassNotation/formatted.pug
@@ -0,0 +1,5 @@
+div(class="class")
+div(
+  class="class",
+  attribute="value"
+)
diff --git a/tests/options/pugWrapAttributesThreshold/withAttributeClassNotation/unformatted.pug b/tests/options/pugWrapAttributesThreshold/withAttributeClassNotation/unformatted.pug
new file mode 100644
index 0000000..3371c89
--- /dev/null
+++ b/tests/options/pugWrapAttributesThreshold/withAttributeClassNotation/unformatted.pug
@@ -0,0 +1,2 @@
+div(class="class")
+div(class="class" attribute="value")
diff --git a/tests/options/pugWrapAttributesThreshold/withAttributeClassNotation/with-attribute-class-notation.test.ts b/tests/options/pugWrapAttributesThreshold/withAttributeClassNotation/with-attribute-class-notation.test.ts
new file mode 100644
index 0000000..b137044
--- /dev/null
+++ b/tests/options/pugWrapAttributesThreshold/withAttributeClassNotation/with-attribute-class-notation.test.ts
@@ -0,0 +1,17 @@
+import { compareFiles } from 'tests/common';
+import { describe, expect, it } from 'vitest';
+
+describe('Options', () => {
+  describe('pugWrapAttributesThreshold', () => {
+    it('should count attribute classes toward wrapping if pugClassNotation === attribute', async () => {
+      const { actual, expected } = await compareFiles(import.meta.url, {
+        formatOptions: {
+          pugWrapAttributesThreshold: 1,
+          pugClassNotation: 'attribute',
+        },
+      });
+
+      expect(actual).toBe(expected);
+    });
+  });
+});