Feature Request: Make the separation of attributes with a comma an option

[marlonmarcello]

Hi @Shinigami92 !
As we previously talked on #11 I am making a feature request!

Looking through Pug's docs, they never use comma to separate the attributes on tags.

I think we should keep it consistent and have that same behaviour as default.

I, personally would remove the comma separation completely but I can see how it can be nice for other users, so I propose we make it an option.

The option could be called useCommaAttributes, or something similar, it would be a boolean that if set to true would have the parser add commas to separate the attributes on tags.

Thanks again, and please let me know your thoughts.

[Shinigami92]

The documentation says the following:

Tag attributes look similar to HTML (with optional commas), but their values are just regular JavaScript.

So there are two different possibilities:

// Using commas
div(class='div-class', (click)='play()')
// Using quoted attributes
div(class='div-class' '(click)'='play()')

However, there is a problem:
The pug source code needs to be parsed by the lexer and therefore something like
div(class='div-class' (click)='play()') [angular] will result in an SyntaxError.
Something like div(class='div-class' @click='play()') [vue] will pass.

I'm currently not sure, if we can only have a boolean or we need a string like 'as-needed' | 'always' | 'use-quote-attributes'.

Do you think we should let the developer decide to use something like as-needed or use-quote-attributes?
Do you have better names?

[marlonmarcello]

These are all great points @Shinigami92 I had never used pug with Angular, Vue or any framework with templates. I've been using it for years to create static website templates and those are great use cases.

I really like your suggestion, I feel like 'as-needed' though, will create inconsistencies which is what prettier tries to avoid. I was looking at pug-lint to see how they deal with this, but it seems that they use commas as well.

I am questioning myself now, haha
Maybe commas should be the default, if they help solve most problems then that is great.

[marlonmarcello]

What do you think of this. Having an option, called attributeSeparator, or similar, that defaults to comma, which is what you have right now and seems to be the best option, but you could use none|false for when you don't want anything, or 'quotes' for when you want quoted attributes.

[Shinigami92]

So

{
  "attributeSeparator": true,
  "attributeSeparator": "comma",

  "attributeSeparator": false,
  "attributeSeparator": "none",

  "attributeSeparator": "quotes"
}

And what if someone thinks "Hey, I'm happy and want colons" 😄?

{"attributeSeparator": "colons"}

🤔

I think we should find an option that does not need to be documented, strictly clear and simple.
The inspiration of as-needed is coming from tools like:

• Prettier: Quote Props
• TSLint: object-literal-key-quotes
• eslint: quote-props

[marlonmarcello]

@Shinigami92 took a look at those and you are right again. We start giving too many options, things will get out of control. I like your original idea, just as-needed and always as options.
Simple, and it will cover all use cases.

[Shinigami92]

@marlonmarcello Currently working on this feature.
Would like to let you know that I have found a DANGEROUS 🚨 example

hello-framework([name]="name" [version]="version", (release)="onVersionRelease()")
// converts to
<hello-framework [name]="version" (release)="onVersionRelease()"></hello-framework>

Note the merged vesrion into name

hello-framework([name]="name", [version]="version", (release)="onVersionRelease()")
// converts to
<hello-framework [name]="name", [version]="version", (release)="onVersionRelease()"></hello-framework>

[Shinigami92]

Please give me some examples, so I can add it to the tests

[marlonmarcello]

Oh man. To be fair, on the first example it's a mix of commas and not commas, I feel like that is a job for a linter to fix? What do you think?

Some examples:

Multi line attributes:

nav-component(
  locale-relative-redirect="true"
  highlight="home"
  pin="false"
)

Mixin plus attributes:

+anchor({ href: "contact/"}).extra-class(data-popup="true")

Variable attributes:

.wrapper(data-nav=true data-next="Next" data-prev="Prev" data-slides=4 data-arr=[1,2,3,4,5,6] data-obj={att: 1, attr2: 2})

[Shinigami92]

Um... Should data-nav=true be shortened to data-nav?

Can you give me also the mixin to anchor?

[marlonmarcello]

I don't think it should be, the PUG compiler will deal with that, because for example, if you set your doctype to doctype html when you pass false to an attribute like data-nav=false pug will remove that attribute from the final HTML file. So I think we should let the compiler decide.

This is the anchor mixin

mixin anchor({ href, isExternal })
  if !isExternal
    - href = `${LOCALE_ROOT}/${href}`
  a(
    href=`${href}`
  )&attributes(attributes)
    if block
      block

[Shinigami92]

... Have to handle &attributes, but in another branch

[marlonmarcello]

Yeah, it's a special case.

[Shinigami92]

OK ... it works ... ^^ '
But I don't want to publish it right now, because I'm not satisfied with the current output
I think it needs to format more, like the array should be [1, 2, 3, 4, 5, 6] and so on
It's too late, I'll work on it tomorrow when I have time

If you want, you can clone the repo, use yarn link, test the issue-13-branch, and then give more feedback if something is not like you expected

Edit: and you need to perform yarn build in the project

[marlonmarcello]

Amazing work @Shinigami92 ! Thanks a lot