Skip to content

Rule proposition: Template Indentation #46

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

Closed
filipalacerda opened this issue Jun 23, 2017 · 17 comments
Closed

Rule proposition: Template Indentation #46

filipalacerda opened this issue Jun 23, 2017 · 17 comments

Comments

@filipalacerda
Copy link
Contributor

It would be great to have the ability of having only one style for the template/render function HTML, by enforcing an indentation style.

It could be configured by setting the option to tab for tabs or a positive number for spaces.

Proposal:

  1. For tabs 'vue/indent': 'tab'
<application>
	<component />
</application>
  1. For 2 spaces: 'vue/indent': 2
<application>
  <component />
</application>
  1. No indentation: 'vue/indent': 0
<application>
<component />
</application>
@filipalacerda
Copy link
Contributor Author

@mysticatea should I open a PR or should I wait for feedback on this? Not sure what is the process here :) Can you please help me? Thanks! (The same regarding #47 )

@mysticatea
Copy link
Member

I'm sorry that I had left no comment.

Of course, I agree that this plugin should have indentation rule!
However, I have some points which need discussion:

  1. vue/html-indent instead of vue/indent might be better since <template> supports several formats (e.g. HTML, Pug, ...) which have different indentation rule.
  2. About attributes.
<!-- Attribute names should indent 1 level. -->
<div
    class="">
<!-- Attribute eq should indent 1 level. -->
<div class
    ="">
<div
    class
        ="">
<!-- Attribute values should indent 1 level. -->
<div class=
    "">
<div
    class
        =
            "">
<!-- The content of attribute values should indent 1 level. -->
<div
    :class="
        value
    ">
<div
    :class
        =
            "
                value
            ">
  1. About closing brackets.
<!-- Closing brackets should have the same level as the opening brackets. -->
<div
    class=""
>
<div class=""
>
<!-- Probably somebody want to indent 1 level. -->
<div
    class=""
    >
<div class=""
    >
  1. About expressions.
<!-- The content of expression containers should indent 1 level. -->
{{
    a + b + c
}}
<!-- Expressions should have the same indentation rule as ESLint core indent rule. -->
{{
    a + b +
        c
}}

@filipalacerda
Copy link
Contributor Author

Thanks for the feedback @mysticatea.

I fully agree with 1. and 4.
I have some concerns regarding 2. and 3. because I think they would go against this one #47 🤔

Not against the rule itself, but agains the idea of having each line break meaning a new prop.
What do you think?

@mysticatea
Copy link
Member

mysticatea commented Jun 28, 2017

because I think they would go against this one #47

I don't think so.
This is the rule about spaces at the beginning of lines. Both 2. and 3. are as well. Those don't mention line breaks, enforce indentation if attributes are at the beginning of lines.

@michalsnik
Copy link
Member

michalsnik commented Jun 29, 2017

Let us know if you're going to start working on this @filipalacerda then we'll set in progress label so that no one will duplicate your effort :)

@filipalacerda
Copy link
Contributor Author

filipalacerda commented Jun 29, 2017

@michalsnik I am starting this one #47 can you set that label on that one instead? Thanks!

This one still seems to be in consideration while #47 is already accepted! 🎉

@michalsnik
Copy link
Member

michalsnik commented Jun 29, 2017

Sure thing @filipalacerda, done :) Good luck! Looking forward to review your PR 🚀

@chrisvfritz
Copy link
Contributor

@mysticatea If I understand the comment correctly, I agree with @filipalacerda regarding 2 and 3. I think of HTML elements and attributes the same way I think about objects in JavaScript. So for example, the equivalent of:

<div class
  ="">

Might be this in JS:

{ foo
  : '' }

Which is technically valid, but I think most people would agree it's very ugly and not intuitive to read. Instead, these are the only two styles I would probably find acceptable:

<div class="">
<div 
  class=""
>

Which again, in JS would be:

{ foo: '' }
{
  foo: ''
}

The 2nd one might seem strange in HTML at first, but I think it's only because many developers (myself included) haven't cared too much about the readability of their HTML in the past. We're just used to keeping each tag on its own line, no matter how many attributes - so > on its own line can look very strange at first, even though it's really no different from } on its own line.

@mysticatea
Copy link
Member

@chrisvfritz I agree with you for readability, but the place of line breaks are not related to indentation rule. The indentation rule checks only spacing at the beginning of lines even if developers write line breaks anywhere.

@mysticatea
Copy link
Member

I'd like to write this rule after I have done rewriting vue-eslint-parser.

@chrisvfritz
Copy link
Contributor

@mysticatea Sounds good - I don't have a strong opinion about where it's enforced, but I do think that somewhere we should be able to flag examples like these:

<div
  class=""
  >
<div
  class="">
<div
  class
    =""
>

and ideally, 😍 autofix them to either:

<div class="">

Or:

<div
  class=""
>

I'm thinking this would depend on the settings for indentation and max number of inline attributes per line. If the max inline attributes is 0, then it would autofix to the 2nd example, otherwise it would autofix to the 1st.

Does that make sense?

@mysticatea
Copy link
Member

mysticatea commented Jul 19, 2017

@chrisvfritz The indentation rule does not relate to line breaks. The indentation rule checks only spacing at the beginning of lines even if developers write line breaks anywhere. Also, the indentation rule should fix only spacing at the beginning of lines, so it doesn't touch line breaks. One rule does one thing, it's the philosophy of ESLint.

#47 is one of the rules which handle line breaks.
The combination of those rules, it will provide your ideal fixing eventually.

@chrisvfritz
Copy link
Contributor

@mysticatea Excellent! Like I said, I don't have an opinion on where we enforce that style - as long as we can somewhere. 🙂

@posva
Copy link
Member

posva commented Jul 28, 2017

I was wondering if this was the good thread to talk about attributes alignement:

<!-- attributes vertically aligned to start -->
<div a="a"
     b="b"
>

vs

<!-- 1 indent (tab, spaces, whatever) -->
<div a="a"
    b="b"
>

(which is similar to what is needed right now)

We can also do

<!-- 1 indent (tab, spaces, whatever) -->
<div
    a="a"
    b="b"
>

@chrisvfritz
Copy link
Contributor

@posva What would you think about only allowing your last example?

<div
  a="a"
  b="b"
>

I don't think any other strategy is as consistently readable. For example, the others fall apart with long component names:

<MySuperLongComponentName a="a"
  b="b"
>
<MySuperLongComponentName a="a"
                          b="b"
>

For the latter, I'd even argue that it's a waste of programmer time lining up that indentation. And it's inconsistent with how we write code more generally. If someone formatted a function like below, you might question their sanity. 😅

function myFunc() { var greeting = 'hi'
                    alert(greeting)
}

@posva
Copy link
Member

posva commented Jul 29, 2017

😂 I agree
I started doing that because my editor did it automatically. It works well for short names only, as you point out.
Having one extra line for better redability is a fair tradeoff 💯

@selfup
Copy link

selfup commented Aug 31, 2017

@chrisvfritz It seems we think a bit a like 😂

https://gitlab.com/gitlab-org/gitlab-ce/issues/37217#note_39039029

I really like the idea of treating it like code 👍

mysticatea added a commit that referenced this issue Sep 7, 2017
michalsnik pushed a commit that referenced this issue Oct 15, 2017
* New: `vue/html-indent` rule (fixes #46)

* remove garbages

* add some tests for comma in mustache

* remove switchCase option from docs

* add tests for mix of text and mustache
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

6 participants