Fix class attributes not counting towards attribute wrapping

[MrHaila]

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':

div(class="class" attribute="value")

Output:

// Still on one line (bad)
div(class="class" attribute="value")

Expected output:

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 👋

[Shinigami92]

It looks like this also affects the id (case 'id'). If I correctly understand it, could you also add to the tests an example that uses id="..." and/or #...?

So do we also need to implement this for pugIdNotation? sorry for asking 😅 I was not effectively working on the code for around a year 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

[MrHaila]

Thanks for the kind words and for fast-tracking a new release. This should be good for now 🙏