Skip to content

Add rule max-attributes-per-line (resolves #47) #60

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 16 commits into from
Aug 15, 2017
Merged
Show file tree
Hide file tree
Changes from 4 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
121 changes: 121 additions & 0 deletions docs/rules/max-attributes-per-line.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
# Define the number of attributes allows per line (max-attributes-per-line)

Limits the maximum number of attributes/properties per line to improve readability.


## Rule Details
Copy link
Contributor

Choose a reason for hiding this comment

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

## :book: Rule Details


This rule aims to enforce a number of attributes per line in templates.
It checks all the elements in a template and verifies that the number of attributes per line does not exceed the defined maximum.
An attribute is considered to be in a new line when there is a line break between two attributes.

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).

Examples of **incorrect** code for this rule:
Copy link
Contributor

Choose a reason for hiding this comment

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

:-1: Examples of **incorrect** code for this rule:


```js
Copy link
Contributor

@armano2 armano2 Jul 30, 2017

Choose a reason for hiding this comment

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

```html

<component lorem="1" ipsum="2" dolor="3" sit="4">
</component>

<component
lorem="1" ipsum="2"
dolor="3"
sit="4"
>
</component>
```

Examples of **correct** code for this rule:
Copy link
Contributor

Choose a reason for hiding this comment

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

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


```js
Copy link
Contributor

@armano2 armano2 Jul 30, 2017

Choose a reason for hiding this comment

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

```html

<component lorem="1" ipsum="2" dolor="3">
</component>

<component
lorem="1"
ipsum="2"
dolor="3"
sit="4"
>
</component>

```

### Options
Copy link
Contributor

Choose a reason for hiding this comment

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

## :wrench: Options


```
{
"vue/max-attributes-per-line": ["error", {
"firstline": 1,
"singleline": 2,
"multiline": 1
}]
}
```

### `firstline`
For multi-line declarations, defines how many attributes are acceptable to be put in the first line. (Default 0)

Example of **incorrect** code for this setting:
Copy link
Contributor

Choose a reason for hiding this comment

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

:-1: Examples of **incorrect** code for this rule:

```js
Copy link
Contributor

@armano2 armano2 Jul 30, 2017

Choose a reason for hiding this comment

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

```html

// ["error", { "firstline": 1, "singleline": 2, "multiline": 1}]
<component
foo="John" bar="Smith"
foobar={5555555}>
</component>;
```

Example of **correct** code for this setting:
```js
Copy link
Contributor

@armano2 armano2 Jul 30, 2017

Choose a reason for hiding this comment

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

```html

// ["error", { "firstline": 0, "singleline": 3, "multiline": 1}]
<component
foo="John"
bar="Smith"
foobar={5555555}
>
</component>;
```


### `singleline`
Number of maximum attributes per line when the opening tag is in a single line.

Example of **incorrect** code for this setting:
```js
Copy link
Contributor

@armano2 armano2 Jul 30, 2017

Choose a reason for hiding this comment

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

```html

// ["error", { "firstline": 1, "singleline": 2, "multiline": 1}]
<component foo="John" bar="Smith" foobar={5555555}></component>;
```

Example of **correct** code for this setting:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

```js
Copy link
Contributor

@armano2 armano2 Jul 30, 2017

Choose a reason for hiding this comment

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

```html

// ["error", { "firstline": 1, "singleline": 3, "multiline": 1}]
<component foo="John" bar="Smith" foobar={5555555}></component>;
```


### `multiline`
Number of maximum attributes per line when a tag is in multiple lines.

Example of **incorrect** code for this setting:
Copy link
Contributor

Choose a reason for hiding this comment

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

:-1: Examples of **incorrect** code for this rule:

```js
Copy link
Contributor

@armano2 armano2 Jul 30, 2017

Choose a reason for hiding this comment

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

```html

// ["error", { "firstline": 1, "singleline": 2, "multiline": 1}]
<component foo="John" bar="Smith"
foobar={5555555}>
</component>;
```

Example of **correct** code for this setting:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

```js
Copy link
Contributor

@armano2 armano2 Jul 30, 2017

Choose a reason for hiding this comment

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

```html

// ["error", { "firstline": 0, "singleline": 3, "multiline": 1}]
<component
foo="John"
bar="Smith"
foobar={5555555}
>
</component>;
```

## When Not To Use It

If you do not want to check the number of attributes declared per line you can disable this rule.

111 changes: 111 additions & 0 deletions lib/rules/max-attributes-per-line.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
/**
* @fileoverview Define the number of attributes allows per line
* @author Filipa Lacerda
*/
'use strict'

// ------------------------------------------------------------------------------
// Rule Definition
// ------------------------------------------------------------------------------
const utils = require('../utils')

module.exports = {
meta: {
docs: {
description: 'Define the number of attributes allows per line',
category: 'Stylistic Issues',
recommended: false
},
fixable: 'code',
Copy link
Member

Choose a reason for hiding this comment

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

It should be fixable: null.
fixable: 'code' means that this rule modifies source code with eslint --fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♀️ forgot about that! Fixed!

schema: [{
type: 'object',
properties: {
'firstline': {
type: 'number',
minimum: 0
},
'singleline': {
type: 'number',
minimum: 1
},
'multiline': {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to change this schema to https://jsonschemalint.com/#/version/draft-06/markup/json?gist=a6a3592eaf8365d069031288f29e5445

  • That schema allows {singleline: 1, multiline: 3} if somebody don't mind {allowFirstLine: false}. It's simple.
  • On the other hand, that schema allows {singleline: {max: 1}, multiline: {max: 3}} if somebody want to write as details. It's more consistent than {singleline: 1, multiline: {max: 3}}.

type: 'number',
minimum: 1
}
}
}]
},

create: function (context) {
const configuration = context.options[0]
const multilineMaximum = configuration.multiline
const singlelinemMaximum = configuration.singleline
Copy link
Member

Choose a reason for hiding this comment

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

Documents say the options have each default value. But this looks to not have default values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mysticatea
I am looking at the docs and I am a little lost here. I thought they were assumed by default, meaning, if no options were provided they would fallback to the ones in the schema.

Should I do something like:
const configuration = context.options[0] || { firstline: 0, singleline: 1, multiline: 1}
?

Copy link
Member

@michalsnik michalsnik Jul 16, 2017

Choose a reason for hiding this comment

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

I think it should be something like:

const configuration = context.options[0] || {}
const multilineMaximum = configuration.multiline || 1
const singlelinemMaximum = configuration.singleline || 1

so you can pass only multiline option and still get fallback for singleline

const canHaveFirstLine = configuration.firstline !== 0
Copy link
Member

Choose a reason for hiding this comment

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

I think boolean or string option is more proper in this case.
How about the below?

A: Simple boolean option.

{
    "singleline": 3,
    "multiline": 1,
    "allowFirstLine": false // or true
}

B: I think that the first attribute position is a taste for each people. It might be good if people can specify the position. But I'm not sure it's a responsibility of this rule.

{
    "singleline": 3,
    "multiline": 1,
    "firstAttributeOfMultiline": "newline" // or "beside"
}

C: I'm wondering that it's a setting of multiline. Should it locate in the multiline?

{
    "singleline": 3,
    "multiline": {"max": 1, "allowFirstLine": false}
}

Copy link
Contributor Author

@filipalacerda filipalacerda Jul 13, 2017

Choose a reason for hiding this comment

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

It is only for multiline, I will go with C, what do you think @mysticatea ?

Copy link
Member

Choose a reason for hiding this comment

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

I'd go with C but still keep it as number, e.g.:

{
    "singleline": 3,
    "multiline": {"max": 1, "maxInFirstLine": 2}
}

So it's possible to have two properties in the first line but the rest in new lines in capacity of 1 per line, like here:

<input name="someField" class="input"
  v-model="message"
  placeholder="Lorem ipsum"
>

What do you think @filipalacerda @mysticatea ?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think I get it know @mysticatea. You meant that the value for a singleline option should be considered the max for the first line, when allowFirstLine is true? If that's the case I'm all for it.

Copy link
Member

Choose a reason for hiding this comment

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

@michalsnik This is suggestion which is based on this line of the implementation: const canHaveFirstLine = configuration.firstline !== 0. It uses multiline.max for the first line if allowFirstLine: true.


utils.registerTemplateBodyVisitor(context, {
'VStartTag' (node) {
const numberOfAttributes = node.attributes.length

if (!numberOfAttributes) return

if (isSingleLine(node) && numberOfAttributes > singlelinemMaximum) {
context.report({
node,
loc: node.loc,
Copy link
Member

Choose a reason for hiding this comment

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

I still think better if this rule reports the violated attributes instead of elements which have the violated attributes.
This is consistent with the core rules which have similar functionally.

/*eslint 
    max-statements-per-line: error,
    newline-per-chained-call: error,
    object-property-newline: error,
    array-element-newline: error
 */

aaa(); bbb(); ccc();
test.aaa().bbb().ccc().ddd()
var obj = {aaa:1, bbb:2, ccc:3}
var ary = [1, 2, 3]

image

message: 'It has more than {{singlelinemMaximum}} attributes per line.',
Copy link
Member

Choose a reason for hiding this comment

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

I think better if this rule reports the violated attributes instead of elements which have the violated attributes.
Because editors can indicate more proper locations with red lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! :)

Copy link
Contributor Author

@filipalacerda filipalacerda Jul 13, 2017

Choose a reason for hiding this comment

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

Question:

with the default of 3, if it has more than 3, we would trigger an error saying:"attribute(s) x and z should be in a new line."
But if the multiline max is 1 putting those last ones in a new line will trigger another error.

🤔

What do you think we should do?

Copy link
Member

Choose a reason for hiding this comment

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

I have imaged like that logic. (sorry, I have not run that code.)

Copy link
Member

Choose a reason for hiding this comment

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

Or we can simply put:
There are 3 attributes in this line, but the maximum is 2

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 would rather go with @michalsnik suggestion, since as a user I think it will help me more to know the exact numbers :)

data: {
singlelinemMaximum
}
})
}

if (!isSingleLine(node)) {
if (!canHaveFirstLine && node.attributes[0].loc.start.line === node.loc.start.line) {
context.report({
node,
loc: node.loc,
message: 'Attribute {{propName}} should be on a new line.',
data: {
propName: node.attributes[0].key.name
}
})
}

const propsPerLine = [[node.attributes[0]]]

node.attributes.reduce(function (previous, current) {
Copy link
Member

Choose a reason for hiding this comment

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

  • Use arrow function
  • I'd create groupAttrsByLine or collectAttributes function that would take node.attributes as parameter and return 2d array, using such function in this place would make this code more readable:
const attrsGroupedByLine = groupAttrsByLine(node.attributes);

attrsGroupedByLine
  .filter(attrs => attrs.length > multilineMaximum)
  .forEach(attrs => showErrors(attrs.splice(multilineMaximum), node)

if (previous.loc.end.line === current.loc.start.line) {
propsPerLine[propsPerLine.length - 1].push(current)
} else {
propsPerLine.push([current])
}
return current
})

propsPerLine.forEach(function (attributes) {
Copy link
Member

Choose a reason for hiding this comment

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

You can use arrow function, you don't need the context here

if (attributes.length > multilineMaximum) {
const prop = attributes[multilineMaximum]

context.report({
node,
loc: node.loc,
message: 'Attribute {{propName}} should be on a new line.',
data: {
propName: prop.key.name
}
})
}
})
}
}
})

// ----------------------------------------------------------------------
// Helpers
// ----------------------------------------------------------------------
function isSingleLine (node) {
Copy link
Member

Choose a reason for hiding this comment

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

Consider moving this function to utils

return node.loc.start.line === node.loc.end.line
}
return {}
}
}
91 changes: 91 additions & 0 deletions tests/lib/rules/max-attributes-per-line.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/**
* @fileoverview Define the number of attributes allows per line
* @author Filipa Lacerda
*/
'use strict'

// ------------------------------------------------------------------------------
// Requirements
// ------------------------------------------------------------------------------

const RuleTester = require('eslint').RuleTester
const rule = require('../../../lib/rules/max-attributes-per-line')

// ------------------------------------------------------------------------------
// Tests
// ------------------------------------------------------------------------------

const ruleTester = new RuleTester({
parser: 'vue-eslint-parser',
parserOptions: { ecmaVersion: 2015 }
})

ruleTester.run('max-attributes-per-line', rule, {
valid: [
{
code: `<template><component></component></template>`,
options: [{ singleline: 1, multiline: 1, firstline: 0 }]
},
{
code: `<template><component name="John Doe" age="30" job="Vet"></component></template>`,
options: [{ singleline: 3, multiline: 1, firstline: 0 }]
},
{
code: `<template><component name="John Doe"
age="30">
</component>
</template>`,
options: [{ singleline: 3, multiline: 1, firstline: 1 }]
},
{
code: `<template><component
name="John Doe"
age="30">
</component>
</template>`,
options: [{ singleline: 3, multiline: 1, firstline: 0 }]
},
{
code: `<template><component
name="John Doe" age="30"
job="Vet">
</component>
</template>`,
options: [{ singleline: 3, multiline: 2, firstline: 0 }]
}
Copy link
Member

Choose a reason for hiding this comment

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

I want to see tests with no options (use default values) as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed ✅

],

invalid: [
{
code: `<template><component name="John Doe" age="30" job="Vet"></component></template>`,
options: [{ singleline: 1, multiline: 1, firstline: 0 }],
errors: ['It has more than 1 attributes per line.']
},
{
code: `<template><component name="John Doe"
age="30">
</component>
</template>`,
options: [{ singleline: 3, multiline: 1, firstline: 0 }],
errors: ['Attribute name should be on a new line.']
},
{
code: `<template><component
name="John Doe" age="30"
job="Vet">
</component>
</template>`,
options: [{ singleline: 3, multiline: 1, firstline: 0 }],
errors: ['Attribute age should be on a new line.']
},
{
code: `<template><component
name="John Doe" age="30"
job="Vet" pet="dog" petname="Snoopy">
</component>
</template>`,
options: [{ singleline: 3, multiline: 2, firstline: 0 }],
errors: ['Attribute petname should be on a new line.']
}
]
})