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

feat: add new enforce option to lines-between-class-members #17462

Merged
merged 9 commits into from Sep 2, 2023
185 changes: 182 additions & 3 deletions docs/src/rules/lines-between-class-members.md
Expand Up @@ -69,14 +69,19 @@ class MyClass {

### Options

This rule has a string option and an object option.
This rule has two options, first option can be string or object, second option is object.

String option:
First option can be string `"always"` or `"never"` or an object with a property named `enforce`:

* `"always"`(default) require an empty line after class members
* `"never"` disallows an empty line after class members
* `Object`: An object with a property named `enforce`. The enforce property should be an array of objects, each specifying the configuration for enforcing empty lines between specific pairs of class members.
* **enforce**: You can supply any number of configurations. If a statement pair matches multiple configurations, the last matched configuration will be used. Each object should have the following properties:
snitin315 marked this conversation as resolved.
Show resolved Hide resolved
* **blankLine**: Can be set to either `"always"` or `"never"`, indicating whether a blank line should be enforced between the specified members.
snitin315 marked this conversation as resolved.
Show resolved Hide resolved
* **prev**: Specifies the type of the preceding class member. It can be `"method"` for class methods, `"field"` for class fields, or `"*"` for any class member.
* **next**: Specifies the type of the following class member. It follows the same options as `prev`.

Object option:
Second option is an object with a property named `exceptAfterSingleLine`:

* `"exceptAfterSingleLine": false`(default) **do not** skip checking empty lines after single-line class members
* `"exceptAfterSingleLine": true` skip checking empty lines after single-line class members
Expand Down Expand Up @@ -129,6 +134,146 @@ class Foo{

:::

Examples of **incorrect** code for this rule with the array of configurations option:

::: incorrect

```js
// disallows blank lines between methods
/*eslint lines-between-class-members: [
"error",
{
enforce: [
{ blankLine: "never", prev: "method", next: "method" }
]
},
]*/

class MyClass {
constructor(height, width) {
this.height = height;
this.width = width;
}

fieldA = 'Field A';
#fieldB = 'Field B';

method1() {}

get area() {
return this.method1();
}

method2() {}
}
```

:::

::: incorrect

```js
// requires blank lines around fields, disallows blank lines between methods
/*eslint lines-between-class-members: [
"error",
{
enforce: [
{ blankLine: "always", prev: "*", next: "field" },
{ blankLine: "always", prev: "field", next: "*" },
{ blankLine: "never", prev: "method", next: "method" }
]
},
]*/

class MyClass {
constructor(height, width) {
this.height = height;
this.width = width;
}
fieldA = 'Field A';
#fieldB = 'Field B';
method1() {}

get area() {
return this.method1();
}

method2() {}
}
```

:::

Examples of **correct** code for this rule with the array of configurations option:

::: correct

```js
// disallows blank lines between methods
/*eslint lines-between-class-members: [
"error",
{
enforce: [
{ blankLine: "never", prev: "method", next: "method" }
]
},
]*/

class MyClass {
constructor(height, width) {
this.height = height;
this.width = width;
}

fieldA = 'Field A';

#fieldB = 'Field B';

method1() {}
get area() {
return this.method1();
}
method2() {}
}
```

:::

::: correct

```js
// requires blank lines around fields, disallows blank lines between methods
/*eslint lines-between-class-members: [
"error",
{
enforce: [
{ blankLine: "always", prev: "*", next: "field" },
{ blankLine: "always", prev: "field", next: "*" },
{ blankLine: "never", prev: "method", next: "method" }
]
},
]*/

class MyClass {
constructor(height, width) {
this.height = height;
this.width = width;
}

fieldA = 'Field A';

#fieldB = 'Field B';

method1() {}
get area() {
return this.method1();
}
method2() {}
}
```

:::

Examples of **correct** code for this rule with the object option:

::: correct
Expand All @@ -148,6 +293,40 @@ class Foo{

:::

::: correct

```js
/*eslint lines-between-class-members: [
"error",
{
enforce: [
{ blankLine: "always", prev: "*", next: "method" },
{ blankLine: "always", prev: "method", next: "*" },
{ blankLine: "always", prev: "field", next: "field" }
]
},
{ exceptAfterSingleLine: true }
]*/

class MyClass {
constructor(height, width) {
this.height = height;
this.width = width;
}

fieldA = 'Field A';
#fieldB = 'Field B';
method1() {}
get area() {
return this.method1();
}

method2() {}
}
```

:::

## When Not To Use It

If you don't want to enforce empty lines between class members, you can disable this rule.
Expand Down
103 changes: 101 additions & 2 deletions lib/rules/lines-between-class-members.js
Expand Up @@ -10,6 +10,21 @@

const astUtils = require("./utils/ast-utils");

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------

/**
* Types of class members.
* Those have `test` method to check it matches to the given class member.
* @private
*/
const ClassMemberTypes = {
"*": { test: () => true },
field: { test: node => node.type === "PropertyDefinition" },
method: { test: node => node.type === "MethodDefinition" }
};

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand All @@ -29,7 +44,30 @@ module.exports = {

schema: [
{
enum: ["always", "never"]
anyOf: [
{
type: "object",
properties: {
enforce: {
type: "array",
items: {
type: "object",
properties: {
blankLine: { enum: ["always", "never"] },
prev: { enum: ["method", "field", "*"] },
next: { enum: ["method", "field", "*"] }
},
additionalProperties: false,
required: ["blankLine", "prev", "next"]
}
}
},
additionalProperties: false
snitin315 marked this conversation as resolved.
Show resolved Hide resolved
},
{
enum: ["always", "never"]
}
]
},
{
type: "object",
Expand All @@ -55,6 +93,7 @@ module.exports = {
options[0] = context.options[0] || "always";
options[1] = context.options[1] || { exceptAfterSingleLine: false };

const configureList = typeof options[0] === "object" && options[0].enforce ? options[0].enforce : [];
snitin315 marked this conversation as resolved.
Show resolved Hide resolved
const sourceCode = context.sourceCode;

/**
Expand Down Expand Up @@ -144,6 +183,38 @@ module.exports = {
return sourceCode.getTokensBetween(before, after, { includeComments: true }).length !== 0;
}

/**
* Checks whether the given node matches the given type.
* @param {ASTNode} node The class member node to check.
* @param {string|string[]} type The class member type to check.
snitin315 marked this conversation as resolved.
Show resolved Hide resolved
* @returns {boolean} `true` if the class member node matched the type.
* @private
*/
function match(node, type) {
return ClassMemberTypes[type].test(node);
}

/**
* Finds the last matched configuration from the configureList.
* @param {ASTNode} prevNode The previous statement to match.
* @param {ASTNode} nextNode The current statement to match.
* @returns {Object} The tester of the last matched configure.
snitin315 marked this conversation as resolved.
Show resolved Hide resolved
* @private
*/
function getPaddingType(prevNode, nextNode) {
for (let i = configureList.length - 1; i >= 0; --i) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some tests where multiple config objects match a pair, to confirm that the last match wins? I tried switching this to for (let i = 0; i < configureList.length; ++i) and all tests were still passing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in ea4fb7f

const configure = configureList[i];
const matched =
match(prevNode, configure.prev) &&
match(nextNode, configure.next);

if (matched) {
return configure.blankLine;
}
}
return null;
}

return {
ClassBody(node) {
const body = node.body;
Expand All @@ -159,7 +230,35 @@ module.exports = {
const hasTokenInPadding = hasTokenOrCommentBetween(beforePadding, afterPadding);
const curLineLastToken = findLastConsecutiveTokenAfter(curLast, nextFirst, 0);

if ((options[0] === "always" && !skip && !isPadded) ||
if (configureList.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This condition seems redundant since configureList is always a non-empty array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, removed.

const paddingType = getPaddingType(body[i], body[i + 1]);

if (paddingType === "never" && isPadded) {
context.report({
node: body[i + 1],
messageId: "never",

fix(fixer) {
if (hasTokenInPadding) {
return null;
}
return fixer.replaceTextRange([beforePadding.range[1], afterPadding.range[0]], "\n");
}
});
} else if (paddingType === "always" && !skip && !isPadded) {
context.report({
node: body[i + 1],
messageId: "always",

fix(fixer) {
if (hasTokenInPadding) {
return null;
}
return fixer.insertTextAfter(curLineLastToken, "\n");
}
});
}
} else if ((options[0] === "always" && !skip && !isPadded) ||
(options[0] === "never" && isPadded)) {
context.report({
node: body[i + 1],
Expand Down