-
-
Notifications
You must be signed in to change notification settings - Fork 681
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
Changes from 4 commits
a7d9313
7ef1ca5
6a62bdd
1d34713
c78b20d
56f5f70
ca31f09
ed73ad8
df1207b
39d0743
796611e
982d9f2
2f0492d
5539686
dcb3ee3
dc64c62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
|
||
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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
```js | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
<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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
```js | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
<component lorem="1" ipsum="2" dolor="3"> | ||
</component> | ||
|
||
<component | ||
lorem="1" | ||
ipsum="2" | ||
dolor="3" | ||
sit="4" | ||
> | ||
</component> | ||
|
||
``` | ||
|
||
### Options | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
``` | ||
{ | ||
"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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
```js | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// ["error", { "firstline": 1, "singleline": 2, "multiline": 1}] | ||
<component | ||
foo="John" bar="Smith" | ||
foobar={5555555}> | ||
</component>; | ||
``` | ||
|
||
Example of **correct** code for this setting: | ||
```js | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// ["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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// ["error", { "firstline": 1, "singleline": 2, "multiline": 1}] | ||
<component foo="John" bar="Smith" foobar={5555555}></component>; | ||
``` | ||
|
||
Example of **correct** code for this setting: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
```js | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// ["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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
```js | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// ["error", { "firstline": 1, "singleline": 2, "multiline": 1}] | ||
<component foo="John" bar="Smith" | ||
foobar={5555555}> | ||
</component>; | ||
``` | ||
|
||
Example of **correct** code for this setting: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
```js | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// ["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. | ||
|
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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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': { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||
type: 'number', | ||
minimum: 1 | ||
} | ||
} | ||
}] | ||
}, | ||
|
||
create: function (context) { | ||
const configuration = context.options[0] | ||
const multilineMaximum = configuration.multiline | ||
const singlelinemMaximum = configuration.singleline | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mysticatea Should I do something like: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should be something like:
so you can pass only |
||
const canHaveFirstLine = configuration.firstline !== 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think 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}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd go with
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:
What do you think @filipalacerda @mysticatea ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: |
||
|
||
utils.registerTemplateBodyVisitor(context, { | ||
'VStartTag' (node) { | ||
const numberOfAttributes = node.attributes.length | ||
|
||
if (!numberOfAttributes) return | ||
|
||
if (isSingleLine(node) && numberOfAttributes > singlelinemMaximum) { | ||
context.report({ | ||
node, | ||
loc: node.loc, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. /*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] |
||
message: 'It has more than {{singlelinemMaximum}} attributes per line.', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed! :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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." 🤔 What do you think we should do? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or we can simply put: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 {} | ||
} | ||
} |
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 }] | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.'] | ||
} | ||
] | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.