Skip to content

Style Guide Alignment #205

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

Merged
merged 10 commits into from
Nov 16, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
node_modules
lib/recommended-rules.js
lib/*-rules.js
coverage
.nyc_output
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
/tests/integrations/*/node_modules
/node_modules
/test.*
yarn.lock
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't ignore yarn.lock, we might want to add it to repository

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it'd be safer to add it to .gitignore until an explicit decision to include it had been made, at which time it could be easily removed. Do you disagree?

114 changes: 68 additions & 46 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,87 +75,109 @@ Vue.component('AsyncComponent', (resolve, reject) => {
## :gear: Configs

This plugin provides two predefined configs:
- `plugin:vue/base` - contains necessary settings for this plugin to work properly
- `plugin:vue/recommended` - extends base config with recommended rules (the ones with check mark :white_check_mark: in the table below)
- `plugin:vue/base` - Settings and rules to enable correct ESLint parsing
- `plugin:vue/essential` - Above, plus rules to prevent errors or unintended behavior
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those configs seems very nice, but I'm not sure we should group them like this in readme. Current groups are common in eslint ecosystem (take a look at original rules https://eslint.org/docs/rules/). Plus later we will probably have some rules that we don't want to enable in any config and give the user full freedom in deciding whether he needs it or not. With the proposed granulation we would also have to add another category - which may lead to unnecessary churn. Also I'm not sure whether maintaining 4 configs is a way to go. I'd rather keep it simple stupid and provide only base and recommended config. What do you think @chrisvfritz @mysticatea ?

Copy link
Contributor Author

@chrisvfritz chrisvfritz Oct 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current groups are common in eslint ecosystem (take a look at original rules eslint.org/docs/rules).

I agree it makes sense to stick with ESLint conventions wherever there's not a compelling reason to stray, but I did see some compelling reasons here.

First, I'm thinking that most people will probably discover the ESLint plugin through the style guide, so I saw a strong benefit in not forcing users to context switch after arriving here. If they want to enforce priority A rules, they'll know within seconds exactly what to do. There will be no wondering, "Wait, is Possible Errors what I want to enforce Priority A rules?"

For users who have never seen the style guide, but are familiar with ESLint conventions, I don't think the new name will cause any confusion - but perhaps there's something I'm not foreseeing. What do you think?

Second, I think there's value in the increased granularity. "Stylistic Issues" is too broad in my opinion, which is why I split stylistic rules between Priority B and C. For Priority B, there's one or more possible conventions that offer clear benefits over the alternatives. For Priority C, there are really many good options and the primary value is just in picking something and sticking to it.

Finally, I moved jsx-uses-vars out of "Variables" because I just don't find that a useful category, despite ESLint's recommendation. Their rules in that category include a mix of possible errors (e.g. no-use-before-define) and stylistic issues (e.g. init-declarations) and this is the only language feature (apart from ES6) that receives special treatment. I also feel that jsx-uses-vars only incidentally has to do with variables - it's really a parsing extension that allows ESLint to work correctly, which is why I added it to base.

Plus later we will probably have some rules that we don't want to enable in any config and give the user full freedom in deciding whether he needs it or not.

I'm not sure I understand this one. Don't users have the same amount of freedom either way, since they can override whatever they want? In terms of ease of use, I can't think of a case where more nested rule sets would worsen the user experience or make it more difficult for them to do something. Am I missing something though?

With the proposed granulation we would also have to add another category - which may lead to unnecessary churn.

I might need more context to understand this one, because adding a new category wouldn't be a lot more code nor increase its complexity, if I'm understanding correctly. Can you provide a specific example of what you're thinking might happen?

Also I'm not sure whether maintaining 4 configs is a way to go. I'd rather keep it simple stupid and provide only base and recommended config.

This is another one where I might need more context. The configs are generated with a script, so I don't believe there's any extra maintenance needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I get your point better now @chrisvfritz Thanks for adding more context :)

And regarding last three paragraphs that you quoted:
I was thinking about the case where we want to introduce a rule, but we don't want to enable it in any config by default. Let's say we'd want to give people possibility to use the given rule, but it's not aligned with the official styleguide for some reason.
Maybe it's an edge case, but we had a similar scenario in eslint-plugin-ember.

Adding another group is not a big deal though, so I think that we can move forward with the current state, and iterate over it later on 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalsnik I agree it'd be good to introduce a category like that when the need arises - and I can already think of use cases for it. For example, I like to disallow filters in many projects, because they add one more thing that developers have to learn without enabling them to do anything new. That kind of thing wouldn't fit into any (existing) categories of the style guide though.

- `plugin:vue/strongly-recommended` - Above, plus rules to considerably improve code readability and/or dev experience
- `plugin:vue/recommended` - Above, plus rules to enforce subjective community defaults to ensure consistency

## :bulb: Rules

Rules are grouped by category to help you understand their purpose.
Rules are grouped by priority to help you understand their purpose. The `--fix` option on the command line automatically fixes problems reported by rules which have a wrench :wrench: below.

No rules are enabled by `plugin:vue/base` config. The `plugin:vue/recommended` config enables rules that report common problems, which have a check mark :white_check_mark: below.
<!--RULES_TABLE_START-->

The `--fix` option on the command line automatically fixes problems reported by rules which have a wrench :wrench: below.
### Base Rules (Enabling Correct ESLint Parsing)

<!--RULES_TABLE_START-->
Enforce all the rules in this category, as well as all higher priority rules, with:

### Possible Errors
``` json
"extends": "plugin:vue/base"
```

| | Rule ID | Description |
|:---|:--------|:------------|
| | [jsx-uses-vars](./docs/rules/jsx-uses-vars.md) | prevent variables used in JSX to be marked as unused |


### Priority A: Essential (Error Prevention)

Enforce all the rules in this category, as well as all higher priority rules, with:

``` json
"extends": "plugin:vue/essential"
```

| | Rule ID | Description |
|:---|:--------|:------------|
| | [no-async-in-computed-properties](./docs/rules/no-async-in-computed-properties.md) | disallow asynchronous actions in computed properties |
| | [no-dupe-keys](./docs/rules/no-dupe-keys.md) | disallow duplication of field names |
| :white_check_mark: | [no-parsing-error](./docs/rules/no-parsing-error.md) | disallow parsing errors in `<template>` |
| | [no-duplicate-attributes](./docs/rules/no-duplicate-attributes.md) | disallow duplication of attributes |
| | [no-parsing-error](./docs/rules/no-parsing-error.md) | disallow parsing errors in `<template>` |
| | [no-reserved-keys](./docs/rules/no-reserved-keys.md) | disallow overwriting reserved keys |
| | [no-shared-component-data](./docs/rules/no-shared-component-data.md) | enforce component's data property to be a function |
| | [no-side-effects-in-computed-properties](./docs/rules/no-side-effects-in-computed-properties.md) | disallow side effects in computed properties |
| | [no-template-key](./docs/rules/no-template-key.md) | disallow `key` attribute on `<template>` |
| | [no-textarea-mustache](./docs/rules/no-textarea-mustache.md) | disallow mustaches in `<textarea>` |
| | [no-unused-vars](./docs/rules/no-unused-vars.md) | disallow unused variable definitions of v-for directives or scope attributes |
| | [require-component-is](./docs/rules/require-component-is.md) | require `v-bind:is` of `<component>` elements |
| | [require-render-return](./docs/rules/require-render-return.md) | enforce render function to always return value |
| | [require-v-for-key](./docs/rules/require-v-for-key.md) | require `v-bind:key` with `v-for` directives |
| | [require-valid-default-prop](./docs/rules/require-valid-default-prop.md) | enforce props default values to be valid |
| | [return-in-computed-property](./docs/rules/return-in-computed-property.md) | enforce that a return statement is present in computed property |
| :white_check_mark: | [valid-template-root](./docs/rules/valid-template-root.md) | enforce valid template root |
| :white_check_mark: | [valid-v-bind](./docs/rules/valid-v-bind.md) | enforce valid `v-bind` directives |
| :white_check_mark: | [valid-v-cloak](./docs/rules/valid-v-cloak.md) | enforce valid `v-cloak` directives |
| :white_check_mark: | [valid-v-else-if](./docs/rules/valid-v-else-if.md) | enforce valid `v-else-if` directives |
| :white_check_mark: | [valid-v-else](./docs/rules/valid-v-else.md) | enforce valid `v-else` directives |
| :white_check_mark: | [valid-v-for](./docs/rules/valid-v-for.md) | enforce valid `v-for` directives |
| :white_check_mark: | [valid-v-html](./docs/rules/valid-v-html.md) | enforce valid `v-html` directives |
| :white_check_mark: | [valid-v-if](./docs/rules/valid-v-if.md) | enforce valid `v-if` directives |
| :white_check_mark: | [valid-v-model](./docs/rules/valid-v-model.md) | enforce valid `v-model` directives |
| :white_check_mark: | [valid-v-on](./docs/rules/valid-v-on.md) | enforce valid `v-on` directives |
| :white_check_mark: | [valid-v-once](./docs/rules/valid-v-once.md) | enforce valid `v-once` directives |
| :white_check_mark: | [valid-v-pre](./docs/rules/valid-v-pre.md) | enforce valid `v-pre` directives |
| :white_check_mark: | [valid-v-show](./docs/rules/valid-v-show.md) | enforce valid `v-show` directives |
| :white_check_mark: | [valid-v-text](./docs/rules/valid-v-text.md) | enforce valid `v-text` directives |


### Best Practices

| | Rule ID | Description |
|:---|:--------|:------------|
| :wrench: | [html-end-tags](./docs/rules/html-end-tags.md) | enforce end tag style |
| | [no-async-in-computed-properties](./docs/rules/no-async-in-computed-properties.md) | disallow asynchronous actions in computed properties |
| :white_check_mark: | [no-confusing-v-for-v-if](./docs/rules/no-confusing-v-for-v-if.md) | disallow confusing `v-for` and `v-if` on the same element |
| | [no-duplicate-attributes](./docs/rules/no-duplicate-attributes.md) | disallow duplication of attributes |
| | [no-side-effects-in-computed-properties](./docs/rules/no-side-effects-in-computed-properties.md) | disallow side effects in computed properties |
| :white_check_mark: | [no-textarea-mustache](./docs/rules/no-textarea-mustache.md) | disallow mustaches in `<textarea>` |
| | [order-in-components](./docs/rules/order-in-components.md) | enforce order of properties in components |
| :white_check_mark: | [require-component-is](./docs/rules/require-component-is.md) | require `v-bind:is` of `<component>` elements |
| | [require-default-prop](./docs/rules/require-default-prop.md) | require default value for props |
| | [require-prop-types](./docs/rules/require-prop-types.md) | require type definitions in props |
| :white_check_mark: | [require-v-for-key](./docs/rules/require-v-for-key.md) | require `v-bind:key` with `v-for` directives |
| | [this-in-template](./docs/rules/this-in-template.md) | enforce usage of `this` in template |


### Stylistic Issues
| | [valid-template-root](./docs/rules/valid-template-root.md) | enforce valid template root |
| | [valid-v-bind](./docs/rules/valid-v-bind.md) | enforce valid `v-bind` directives |
| | [valid-v-cloak](./docs/rules/valid-v-cloak.md) | enforce valid `v-cloak` directives |
| | [valid-v-else-if](./docs/rules/valid-v-else-if.md) | enforce valid `v-else-if` directives |
| | [valid-v-else](./docs/rules/valid-v-else.md) | enforce valid `v-else` directives |
| | [valid-v-for](./docs/rules/valid-v-for.md) | enforce valid `v-for` directives |
| | [valid-v-html](./docs/rules/valid-v-html.md) | enforce valid `v-html` directives |
| | [valid-v-if](./docs/rules/valid-v-if.md) | enforce valid `v-if` directives |
| | [valid-v-model](./docs/rules/valid-v-model.md) | enforce valid `v-model` directives |
| | [valid-v-on](./docs/rules/valid-v-on.md) | enforce valid `v-on` directives |
| | [valid-v-once](./docs/rules/valid-v-once.md) | enforce valid `v-once` directives |
| | [valid-v-pre](./docs/rules/valid-v-pre.md) | enforce valid `v-pre` directives |
| | [valid-v-show](./docs/rules/valid-v-show.md) | enforce valid `v-show` directives |
| | [valid-v-text](./docs/rules/valid-v-text.md) | enforce valid `v-text` directives |


### Priority B: Strongly Recommended (Improving Readability)

Enforce all the rules in this category, as well as all higher priority rules, with:

``` json
"extends": "plugin:vue/strongly-recommended"
```

| | Rule ID | Description |
|:---|:--------|:------------|
| :wrench: | [attribute-hyphenation](./docs/rules/attribute-hyphenation.md) | enforce attribute naming style in template |
| :wrench: | [html-end-tags](./docs/rules/html-end-tags.md) | enforce end tag style |
| :wrench: | [html-indent](./docs/rules/html-indent.md) | enforce consistent indentation in `<template>` |
| | [html-quotes](./docs/rules/html-quotes.md) | enforce quotes style of HTML attributes |
| :wrench: | [html-self-closing](./docs/rules/html-self-closing.md) | enforce self-closing style |
| | [max-attributes-per-line](./docs/rules/max-attributes-per-line.md) | enforce the maximum number of attributes per line |
| :wrench: | [mustache-interpolation-spacing](./docs/rules/mustache-interpolation-spacing.md) | enforce unified spacing in mustache interpolations |
| :wrench: | [name-property-casing](./docs/rules/name-property-casing.md) | enforce specific casing for the name property in Vue components |
| :wrench: | [no-multi-spaces](./docs/rules/no-multi-spaces.md) | disallow multiple spaces |
| | [require-default-prop](./docs/rules/require-default-prop.md) | require default value for props |
| | [require-prop-types](./docs/rules/require-prop-types.md) | require type definitions in props |
| :wrench: | [v-bind-style](./docs/rules/v-bind-style.md) | enforce `v-bind` directive style |
| :wrench: | [v-on-style](./docs/rules/v-on-style.md) | enforce `v-on` directive style |


### Variables
### Priority C: Recommended (Minimizing Arbitrary Choices and Cognitive Overhead)

Enforce all the rules in this category, as well as all higher priority rules, with:

``` json
"extends": "plugin:vue/recommended"
```

| | Rule ID | Description |
|:---|:--------|:------------|
| :white_check_mark: | [jsx-uses-vars](./docs/rules/jsx-uses-vars.md) | prevent variables used in JSX to be marked as unused |
| | [html-quotes](./docs/rules/html-quotes.md) | enforce quotes style of HTML attributes |
| | [no-confusing-v-for-v-if](./docs/rules/no-confusing-v-for-v-if.md) | disallow confusing `v-for` and `v-if` on the same element |
| | [order-in-components](./docs/rules/order-in-components.md) | enforce order of properties in components |
| | [this-in-template](./docs/rules/this-in-template.md) | enforce usage of `this` in template |

### Deprecated

Expand Down
24 changes: 4 additions & 20 deletions docs/rules/attribute-hyphenation.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,41 +15,25 @@ Default casing is set to `always`
:+1: Examples of **correct** code`:

```html
<template>
<foo my-prop="prop">
<a onClick="return false"></a>
</foo>
</template>
<MyComponent my-prop="prop"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MyComponent is recommended? my-component?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aladdin-add See this rule in the style guide.

```

:-1: Examples of **incorrect** code`:

```html
<template>
<foo myProp="prop">
<a onClick="return false"></a>
</foo>
</template>
<MyComponent myProp="prop"/>
```

### `"never"` - Don't use hyphenated name. (It errors on hyphens except `data-` and `aria-`.)

:+1: Examples of **correct** code`:

```html
<template>
<foo myProp="prop">
<a onClick="return false"></a>
</foo>
</template>
<MyComponent myProp="prop"/>
```

:-1: Examples of **incorrect** code`:

```html
<template>
<foo my-prop="prop">
<a onClick="return false"></a>
</foo>
</template>
<MyComponent my-prop="prop"/>
```
29 changes: 10 additions & 19 deletions docs/rules/html-end-tags.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,30 +17,21 @@ This rule reports the following elements:
:-1: Examples of **incorrect** code for this rule:

```html
<template>
<div>
<div>
<p>
<p>
<input></input>
<br></br>
</div>
</template>
<div>
<p>
<p>
<input></input>
<br></br>
```

:+1: Examples of **correct** code for this rule:

```html
<template>
<div>
<div></div>
<p></p>
<p></p>
<div />
<input>
<br>
</div>
</template>
<div></div>
<p></p>
<p></p>
<input>
<br>
```

## :wrench: Options
Expand Down
2 changes: 1 addition & 1 deletion docs/rules/html-indent.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ This rule enforces a consistent indentation style in `<template>`. The default s
}
```

- `type` (`number | "tab"`) ... The type of indentation. Default is `4`. If this is a number, it's the number of spaces for one indent. If this is `"tab"`, it uses one tab for one indent.
- `type` (`number | "tab"`) ... The type of indentation. Default is `2`. If this is a number, it's the number of spaces for one indent. If this is `"tab"`, it uses one tab for one indent.
- `attribute` (`integer`) ... The multiplier of indentation for attributes. Default is `1`.
- `closeBracket` (`integer`) ... The multiplier of indentation for right brackets. Default is `0`.
- `ignores` (`string[]`) ... The selector to ignore nodes. The AST spec is [here](https://github.com/mysticatea/vue-eslint-parser/blob/master/docs/ast.md). You can use [esquery](https://github.com/estools/esquery#readme) to select nodes. Default is an empty array.
Expand Down
28 changes: 6 additions & 22 deletions docs/rules/html-quotes.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,43 +15,27 @@ This rule reports the quotes of attributes if it is different to configured quot
:-1: Examples of **incorrect** code for this rule:

```html
<template>
<div>
<img src='./logo.png'>
<img src=./logo.png>
</div>
</template>
<img src='./logo.png'>
<img src=./logo.png>
```

:+1: Examples of **correct** code for this rule:

```html
<template>
<div>
<img src="./logo.png">
</div>
</template>
<img src="./logo.png">
```

:-1: Examples of **incorrect** code for this rule with `"single"` option:

```html
<template>
<div>
<img src="./logo.png">
<img src=./logo.png>
</div>
</template>
<img src="./logo.png">
<img src=./logo.png>
```

:+1: Examples of **correct** code for this rule with `"single"` option:

```html
<template>
<div>
<img src='./logo.png'>
</div>
</template>
<img src='./logo.png'>
```

## :wrench: Options
Expand Down
49 changes: 21 additions & 28 deletions docs/rules/html-self-closing.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@

In Vue.js template, we can use either two styles for elements which don't have their content.

1. `<your-component></your-component>`
2. `<your-component />` (self-closing)
1. `<YourComponent></YourComponent>`
2. `<YourComponent/>` (self-closing)

Self-closing is simple and shorter, but it's not supported in raw HTML.
This rule helps you to unify the self-closing style.
Expand All @@ -16,20 +16,20 @@ This rule has options which specify self-closing style for each context.

```json
{
"html-self-closing": [2, {
"html": {
"normal": "never",
"void": "never",
"component": "always"
},
"svg": "always",
"math": "always"
}]
"html-self-closing": ["error", {
"html": {
"void": "never",
"normal": "always",
"component": "always"
},
"svg": "always",
"math": "always"
}]
}
```

- `html.normal` (`"never"` by default) ... The style of well-known HTML elements except void elements.
- `html.void` (`"never"` by default) ... The style of well-known HTML void elements.
- `html.normal` (`"always"` by default) ... The style of well-known HTML elements except void elements.
- `html.component` (`"always"` by default) ... The style of Vue.js custom components.
- `svg`(`"always"` by default) .... The style of well-known SVG elements.
- `math`(`"always"` by default) .... The style of well-known MathML elements.
Expand All @@ -45,25 +45,18 @@ Every option can be set to one of the following values:
:-1: Examples of **incorrect** code for this rule:

```html
/*eslint html-self-closing: "error"*/

<template>
<div />
<img />
<your-component></your-component>
<svg><path d=""></path></svg>
</template>
<div></div>
<img/>
<img></img>
<MyComponent/></MyComponent>
<svg><path d=""></path></svg>
```

:+1: Examples of **correct** code for this rule:

```html
/*eslint html-self-closing: "error"*/

<template>
<div></div>
<img>
<your-component />
<svg><path d="" /></svg>
</template>
<div/>
<img>
<MyComponent/>
<svg><path d=""/></svg>
```
Loading