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.

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

max-attributes-per-line defaults to "singleline: 1", should be 3 according to docs #300

Closed
tiltec opened this issue Dec 21, 2017 · 13 comments

Comments

@tiltec
Copy link

tiltec commented Dec 21, 2017

Tell us about your environment

  • ESLint Version: any
  • eslint-plugin-vue Version: master
  • Node Version: any

What did you do? Please include the actual source code causing the issue.

I use the strongly-recommended rule that enables max-attributes-per-line. It should default to 3 attributes in the singleline case, according to the docs:

There is a configurable number of attributes that are acceptable in one-line case (default 3), as well as how many attributes are acceptable per line in multi-line case (default 1).

This should be correct:

    <div v-for="item of items" :key="item.id">
      {{ item.name }}
    </div>

There's also a docs inconsistency, as this code is given as incorrect in the docs:

<MyComponent lorem="1" ipsum="2"/>

What did you expect to happen?

Don't get a lint error.

What actually happened?

Got a lint error.

Also noticed by @michalsnik in #77 (comment)

@tiltec
Copy link
Author

tiltec commented Dec 21, 2017

On the other hand, the Vue Style Guide is quite clear that multiple attributes should go on newlines.

So the current default behavior seems correct in this regard, only the docs need updating.

I personally would like to see 3 allowed attributes in the single line case, but I would succumb to the official vue style guide in this regard.

@michalsnik
Copy link
Member

Hey @tiltec, thanks for posting this. Now this topic will have it's proper attention :)

What do you think about this @chrisvfritz? You probably missed my proposition in bunch of other messages in the original issue :D If we're going to change anything here, it has to start with the StyleGuide, so we can align with our source of truth :)

@chrisvfritz
Copy link
Contributor

Since the number of max attributes really depends on the length of those attributes, I think one per line is the only bikeshed-proof choice - unless we include another option to specify max columns before wrapping. On a related note: when Prettier supports HTML, it'll wrap according to max columns. This is in the works, so my preference would probably be to wait for Prettier to take care of it for us, then release a prettier ruleset to disable any rules that overlap with Prettier.

@mysticatea
Copy link
Member

History:

  1. It was 3 at first.
  2. It was changed to 1 at 4.0.0 and forgotten to change the doc.

So I think it's a document issue.

As a side note, I think that we can use prettier from this plugin as similar to https://github.com/prettier/eslint-plugin-prettier if it's pretty (though I have not used prettier).

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Dec 22, 2017

Yeah, I agree it's a documentation issue - sorry if I didn't make that clear. 😅

I'm not sure about using Prettier within this plugin though. Many in the Vue community make ESLint part of their Webpack build process, immediately failing if there's an error. For these users, immediately halting a build on a formatting issue would probably be very frustrating.

I think the strategies of eslint-config-prettier and stylelint-config-prettier are more flexible, so that no matter how Prettier is included in a project, we have a patch ruleset that will avoid any conflicts, deferring to Prettier wherever there's overlap.

@michalsnik
Copy link
Member

Okay, I agree @chrisvfritz. I just wanted to point that it would be good to consider more attributes in one line as it's kind of a common scenario, and I'm also used to change default setting of this rule in each project I'm working on to accept max 3 attrs in one line :)

I'll update docs for now, so they're aligned with the style guide and we can observe if more people will also expect different defaults, if yes - we can iterate the style guide then. WDYT?

@chrisvfritz
Copy link
Contributor

I'm not opposed to changing the default to more than 1 attribute per line, but we'd need to introduce a max columns (i.e. line length) before going multiline - and once we go multiline, I'd want to again require only 1 attribute per line.

The reason is that without a max columns constraint, some attributes will be too long for some people, which means the rule will have failed from preventing us from bikeshedding. 😂

Does that make sense?

@rzaharenkov
Copy link

I am sorry but I think 1 is to strict default. It make my templates stretched in vertical dimension and I need to scroll up and down any time. IMHO 3 make much more sense.

@rzaharenkov
Copy link

BTW, example in vue.js official guide doesn't fit this rule.

@chenxeed
Copy link

chenxeed commented Feb 19, 2018

I agree with @rzaharenkov as well.

Having 2-3 attributes is still considered common and also short so it doesn't have to be strict to be in new line. For example of these cases:

<ul class="lists" v-for="item" v-key="item.id">

<img src="" alt="title" />

<div id="appBox" class="app-wrapper">

<button class="btn btn-submit" @click="submitForm">

Our developers would like to follow the full VueJS recommended standards too, but this rules seems too strict and our HTML template doesn't look really well after most of these elements needs to be in multi-line.

I understand that we can just override the rules with custom max-attribute-per-lines lint rules in the .eslintrc, but again we would prefer to follow the official rules set, just like @tiltec said.

Please reconsider, thank you!

@chrisvfritz
Copy link
Contributor

@rzaharenkov @chenxeed You're both welcome to configure this rule locally as long as you don't mind bikeshedding over cases where even 2 or 3 attributes is too long. 🙂 For what it's worth, I've worked with some clients that have chosen to override it and in every single case, they've eventually changed it back because they got tired of not seeing attributes when troubleshooting and arguing about which cases were appropriate for more than one attribute per line.

FYI though, we'll likely disable this rule by default once Prettier supports HTML, as its line-length-based formatting will create a conflict.

@chenxeed
Copy link

I see. Thanks for clarifying @chrisvfritz ! Having prettier to autolint the max column of HTML will be convenient, and also noted on the possibility of this rule to be removed once prettier supports HTML.

@luis-rocha
Copy link

I personally prefer the way IntelliJ does it, where you can decide whether it wraps or chops depending on if the line is too long.

If the following line were too long then it either wraps

<tag attribute1="value1" attribute2="value2" attribute3="value3"
     attribute4="value4" attribute5="value5" />

or it chops

<tag attribute1="value1"
     attribute2="value2"
     attribute3="value3"
     attribute4="value4"
     attribute5="value5" />

but it's determined by whether the line needs to wrap or not.

If that could be achieved here, it would really make my day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants