From 5a5b598dffd6049be2f6270217f1bf3e04b5536f Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Tue, 3 Dec 2019 00:29:47 +0900 Subject: [PATCH 1/7] Fix: delete comment bug in lines-between-class-members (fixes #12391) --- lib/rules/lines-between-class-members.js | 78 ++++++------------- .../lib/rules/lines-between-class-members.js | 20 +++++ 2 files changed, 45 insertions(+), 53 deletions(-) diff --git a/lib/rules/lines-between-class-members.js b/lib/rules/lines-between-class-members.js index 60332a1b3ca..e30f372a0ec 100644 --- a/lib/rules/lines-between-class-members.js +++ b/lib/rules/lines-between-class-members.js @@ -54,62 +54,33 @@ module.exports = { const sourceCode = context.getSourceCode(); /** - * Checks if there is padding between two tokens - * @param {Token} first The first token - * @param {Token} second The second token - * @returns {boolean} True if there is at least a line between the tokens + * Return the last token among the consecutive comment tokens. "consecutive" means there is no empty line between tokens. + * If there is no any comment token, return token in the parameter. + * @param {Token} token The last token in the member node. + * @returns {Token} The comment token or token in parameter. */ - function isPaddingBetweenTokens(first, second) { - const comments = sourceCode.getCommentsBefore(second); - const len = comments.length; + function getLastConsecutiveTokenAfter(token) { + const after = sourceCode.getTokenAfter(token, { includeComments: true }); - // If there is no comments - if (len === 0) { - const linesBetweenFstAndSnd = second.loc.start.line - first.loc.end.line - 1; - - return linesBetweenFstAndSnd >= 1; - } - - - // If there are comments - let sumOfCommentLines = 0; // the numbers of lines of comments - let prevCommentLineNum = -1; // line number of the end of the previous comment - - for (let i = 0; i < len; i++) { - const commentLinesOfThisComment = comments[i].loc.end.line - comments[i].loc.start.line + 1; - - sumOfCommentLines += commentLinesOfThisComment; - - /* - * If this comment and the previous comment are in the same line, - * the count of comment lines is duplicated. So decrement sumOfCommentLines. - */ - if (prevCommentLineNum === comments[i].loc.start.line) { - sumOfCommentLines -= 1; - } - - prevCommentLineNum = comments[i].loc.end.line; + if (astUtils.isCommentToken(after) && after.loc.start.line - token.loc.end.line <= 1) { + return getLastConsecutiveTokenAfter(after); } + return token; + } - /* - * If the first block and the first comment are in the same line, - * the count of comment lines is duplicated. So decrement sumOfCommentLines. - */ - if (first.loc.end.line === comments[0].loc.start.line) { - sumOfCommentLines -= 1; - } + /** + * Return the fist token among the consecutive comment tokens. "consecutive" means there is no empty line between tokens. + * If there is no any comment token, return token in the parameter. + * @param {Token} token The first token in the member node. + * @returns {Token} The comment token or token in parameter. + */ + function getFirstConsecutiveTokenBefore(token) { + const before = sourceCode.getTokenBefore(token, { includeComments: true }); - /* - * If the last comment and the second block are in the same line, - * the count of comment lines is duplicated. So decrement sumOfCommentLines. - */ - if (comments[len - 1].loc.end.line === second.loc.start.line) { - sumOfCommentLines -= 1; + if (astUtils.isCommentToken(before) && token.loc.start.line - before.loc.end.line <= 1) { + return getFirstConsecutiveTokenBefore(before); } - - const linesBetweenFstAndSnd = second.loc.start.line - first.loc.end.line - 1; - - return linesBetweenFstAndSnd - sumOfCommentLines >= 1; + return token; } return { @@ -120,10 +91,11 @@ module.exports = { const curFirst = sourceCode.getFirstToken(body[i]); const curLast = sourceCode.getLastToken(body[i]); const nextFirst = sourceCode.getFirstToken(body[i + 1]); - const isPadded = isPaddingBetweenTokens(curLast, nextFirst); const isMulti = !astUtils.isTokenOnSameLine(curFirst, curLast); const skip = !isMulti && options[1].exceptAfterSingleLine; - + const beforePadding = getLastConsecutiveTokenAfter(curLast); + const afterPadding = getFirstConsecutiveTokenBefore(nextFirst); + const isPadded = afterPadding.loc.start.line - beforePadding.loc.start.line > 1; if ((options[0] === "always" && !skip && !isPadded) || (options[0] === "never" && isPadded)) { @@ -132,7 +104,7 @@ module.exports = { messageId: isPadded ? "never" : "always", fix(fixer) { return isPadded - ? fixer.replaceTextRange([curLast.range[1], nextFirst.range[0]], "\n") + ? fixer.replaceTextRange([beforePadding.range[1], afterPadding.range[0]], "\n") : fixer.insertTextAfter(curLast, "\n"); } }); diff --git a/tests/lib/rules/lines-between-class-members.js b/tests/lib/rules/lines-between-class-members.js index d1aa8607c0d..3666d947f48 100644 --- a/tests/lib/rules/lines-between-class-members.js +++ b/tests/lib/rules/lines-between-class-members.js @@ -73,6 +73,26 @@ ruleTester.run("lines-between-class-members", rule, { output: "class foo{ bar(){\n}\n\nbaz(){}}", options: ["always", { exceptAfterSingleLine: true }], errors: [alwaysError] + }, { + code: "class foo{ bar(){\n}\n/* comment */\nbaz(){}}", + output: "class foo{ bar(){\n}\n\n/* comment */\nbaz(){}}", + options: ["always", { exceptAfterSingleLine: true }], + errors: [alwaysError] + }, { + code: "class foo{ bar(){}\n\n// comment\nbaz(){}}", + output: "class foo{ bar(){}\n// comment\nbaz(){}}", + options: ["never"], + errors: [neverError] + }, { + code: "class foo{ bar(){}\n\n/* comment */\nbaz(){}}", + output: "class foo{ bar(){}\n/* comment */\nbaz(){}}", + options: ["never"], + errors: [neverError] + }, { + code: "class foo{ bar(){}\n/* comment-1 */\n\n/* comment-2 */\nbaz(){}}", + output: "class foo{ bar(){}\n/* comment-1 */\n/* comment-2 */\nbaz(){}}", + options: ["never"], + errors: [neverError] } ] }); From fd809927e2756cdac8dfc9579b1d049f067019fd Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Fri, 6 Dec 2019 02:11:01 +0900 Subject: [PATCH 2/7] edit comments --- lib/rules/lines-between-class-members.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/rules/lines-between-class-members.js b/lib/rules/lines-between-class-members.js index e30f372a0ec..e525f7cbe0f 100644 --- a/lib/rules/lines-between-class-members.js +++ b/lib/rules/lines-between-class-members.js @@ -54,8 +54,8 @@ module.exports = { const sourceCode = context.getSourceCode(); /** - * Return the last token among the consecutive comment tokens. "consecutive" means there is no empty line between tokens. - * If there is no any comment token, return token in the parameter. + * Return the last token among the consecutive comment tokens, that have no blank line in between. + * If there is no comment token, return token in the parameter. * @param {Token} token The last token in the member node. * @returns {Token} The comment token or token in parameter. */ @@ -69,8 +69,8 @@ module.exports = { } /** - * Return the fist token among the consecutive comment tokens. "consecutive" means there is no empty line between tokens. - * If there is no any comment token, return token in the parameter. + * Return the first token among the consecutive comment tokens, that have no blank line in between. + * If there is no comment token, return token in the parameter. * @param {Token} token The first token in the member node. * @returns {Token} The comment token or token in parameter. */ From b85eaa6a513352a062f2165ce4634dee5a569156 Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Mon, 9 Dec 2019 00:22:43 +0900 Subject: [PATCH 3/7] keep padded comments --- lib/rules/lines-between-class-members.js | 4 ++++ tests/lib/rules/lines-between-class-members.js | 15 +++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/lib/rules/lines-between-class-members.js b/lib/rules/lines-between-class-members.js index e525f7cbe0f..0ada1fd75e3 100644 --- a/lib/rules/lines-between-class-members.js +++ b/lib/rules/lines-between-class-members.js @@ -96,6 +96,7 @@ module.exports = { const beforePadding = getLastConsecutiveTokenAfter(curLast); const afterPadding = getFirstConsecutiveTokenBefore(nextFirst); const isPadded = afterPadding.loc.start.line - beforePadding.loc.start.line > 1; + const hasPaddedComments = sourceCode.commentsExistBetween(beforePadding, afterPadding); if ((options[0] === "always" && !skip && !isPadded) || (options[0] === "never" && isPadded)) { @@ -103,6 +104,9 @@ module.exports = { node: body[i + 1], messageId: isPadded ? "never" : "always", fix(fixer) { + if (hasPaddedComments) { + return null; + } return isPadded ? fixer.replaceTextRange([beforePadding.range[1], afterPadding.range[0]], "\n") : fixer.insertTextAfter(curLast, "\n"); diff --git a/tests/lib/rules/lines-between-class-members.js b/tests/lib/rules/lines-between-class-members.js index 3666d947f48..313c69e2cf8 100644 --- a/tests/lib/rules/lines-between-class-members.js +++ b/tests/lib/rules/lines-between-class-members.js @@ -93,6 +93,21 @@ ruleTester.run("lines-between-class-members", rule, { output: "class foo{ bar(){}\n/* comment-1 */\n/* comment-2 */\nbaz(){}}", options: ["never"], errors: [neverError] + }, { + code: "class foo{ bar(){}\n\n/* comment */\n\nbaz(){}}", + output: null, + options: ["never"], + errors: [neverError] + }, { + code: "class foo{ bar(){}\n\n// comment */\n\nbaz(){}}", + output: null, + options: ["never"], + errors: [neverError] + }, { + code: "class foo{ bar(){}\n/* comment-1 */\n\n/* comment-2 */\n\n/* comment-3 */\nbaz(){}}", + output: null, + options: ["never"], + errors: [neverError] } ] }); From 7ec30b2118527ac7dc8c6c9aca9132d0828016ef Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Wed, 11 Dec 2019 02:45:35 +0900 Subject: [PATCH 4/7] fix to check for semicolon token. --- lib/rules/lines-between-class-members.js | 45 ++++++++++--------- .../lib/rules/lines-between-class-members.js | 28 ++++++++++++ 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/lib/rules/lines-between-class-members.js b/lib/rules/lines-between-class-members.js index 0ada1fd75e3..20c005d8eea 100644 --- a/lib/rules/lines-between-class-members.js +++ b/lib/rules/lines-between-class-members.js @@ -54,33 +54,35 @@ module.exports = { const sourceCode = context.getSourceCode(); /** - * Return the last token among the consecutive comment tokens, that have no blank line in between. - * If there is no comment token, return token in the parameter. - * @param {Token} token The last token in the member node. - * @returns {Token} The comment token or token in parameter. + * Return the last token among the consecutive tokens that have no exceed max line difference in between, before the first token in the next member. + * @param {Token} prevLastToken The last token in the previous member node. + * @param {Token} nextFirstToken The first token in the next member node. + * @param {number} maxLine The maximum number of allowed line difference between consecutive tokens. + * @returns {Token} The last token among the consecutive tokens. */ - function getLastConsecutiveTokenAfter(token) { - const after = sourceCode.getTokenAfter(token, { includeComments: true }); + function findLastConsecutiveTokenAfter(prevLastToken, nextFirstToken, maxLine) { + const after = sourceCode.getTokenAfter(prevLastToken, { includeComments: true }); - if (astUtils.isCommentToken(after) && after.loc.start.line - token.loc.end.line <= 1) { - return getLastConsecutiveTokenAfter(after); + if (after !== nextFirstToken && after.loc.start.line - prevLastToken.loc.end.line <= maxLine) { + return findLastConsecutiveTokenAfter(after, nextFirstToken, maxLine); } - return token; + return prevLastToken; } /** - * Return the first token among the consecutive comment tokens, that have no blank line in between. - * If there is no comment token, return token in the parameter. - * @param {Token} token The first token in the member node. - * @returns {Token} The comment token or token in parameter. + * Return the first token among the consecutive tokens that have no exceed max line difference in between, after the last token in the previous member. + * @param {Token} nextFirstToken The first token in the next member node. + * @param {Token} prevLastToken The last token in the previous member node. + * @param {number} maxLine The maximum number of allowed line difference between consecutive tokens. + * @returns {Token} The first token among the consecutive tokens. */ - function getFirstConsecutiveTokenBefore(token) { - const before = sourceCode.getTokenBefore(token, { includeComments: true }); + function findFirstConsecutiveTokenBefore(nextFirstToken, prevLastToken, maxLine) { + const before = sourceCode.getTokenBefore(nextFirstToken, { includeComments: true }); - if (astUtils.isCommentToken(before) && token.loc.start.line - before.loc.end.line <= 1) { - return getFirstConsecutiveTokenBefore(before); + if (before !== prevLastToken && nextFirstToken.loc.start.line - before.loc.end.line <= maxLine) { + return findFirstConsecutiveTokenBefore(before, prevLastToken, maxLine); } - return token; + return nextFirstToken; } return { @@ -93,10 +95,11 @@ module.exports = { const nextFirst = sourceCode.getFirstToken(body[i + 1]); const isMulti = !astUtils.isTokenOnSameLine(curFirst, curLast); const skip = !isMulti && options[1].exceptAfterSingleLine; - const beforePadding = getLastConsecutiveTokenAfter(curLast); - const afterPadding = getFirstConsecutiveTokenBefore(nextFirst); + const beforePadding = findLastConsecutiveTokenAfter(curLast, nextFirst, 1); + const afterPadding = findFirstConsecutiveTokenBefore(nextFirst, curLast, 1); const isPadded = afterPadding.loc.start.line - beforePadding.loc.start.line > 1; const hasPaddedComments = sourceCode.commentsExistBetween(beforePadding, afterPadding); + const curLineLastToken = findLastConsecutiveTokenAfter(curLast, nextFirst, 0); if ((options[0] === "always" && !skip && !isPadded) || (options[0] === "never" && isPadded)) { @@ -109,7 +112,7 @@ module.exports = { } return isPadded ? fixer.replaceTextRange([beforePadding.range[1], afterPadding.range[0]], "\n") - : fixer.insertTextAfter(curLast, "\n"); + : fixer.insertTextAfter(curLineLastToken, "\n"); } }); } diff --git a/tests/lib/rules/lines-between-class-members.js b/tests/lib/rules/lines-between-class-members.js index 313c69e2cf8..2430483aaa6 100644 --- a/tests/lib/rules/lines-between-class-members.js +++ b/tests/lib/rules/lines-between-class-members.js @@ -40,6 +40,9 @@ ruleTester.run("lines-between-class-members", rule, { "class A{ foo() {}\n/* a */ /* b */\n\nbar() {}}", "class A{ foo() {}/* a */ \n\n /* b */bar() {}}", + "class A {\nfoo() {}\n/* comment */;\n;\n\nbar() {}\n}", + "class A {\nfoo() {}\n// comment */\n\n;\n;\nbar() {}\n}", + "class foo{ bar(){}\n\n;;baz(){}}", "class foo{ bar(){};\n\nbaz(){}}", @@ -108,6 +111,31 @@ ruleTester.run("lines-between-class-members", rule, { output: null, options: ["never"], errors: [neverError] + }, { + code: "class A {\nfoo() {}// comment */;\n;\n/* comment */\nbar() {}\n}", + output: "class A {\nfoo() {}// comment */;\n\n;\n/* comment */\nbar() {}\n}", + options: ["always"], + errors: [alwaysError] + }, { + code: "class A {\nfoo() {}\n/* comment */;\n;\n/* comment */\nbar() {}\n}", + output: "class A {\nfoo() {}\n\n/* comment */;\n;\n/* comment */\nbar() {}\n}", + options: ["always"], + errors: [alwaysError] + }, { + code: "class foo{ bar(){};\nbaz(){}}", + output: "class foo{ bar(){};\n\nbaz(){}}", + options: ["always"], + errors: [alwaysError] + }, { + code: "class foo{ bar(){} // comment \nbaz(){}}", + output: "class foo{ bar(){} // comment \n\nbaz(){}}", + options: ["always"], + errors: [alwaysError] + }, { + code: "class A {\nfoo() {}\n/* comment */;\n;\nbar() {}\n}", + output: "class A {\nfoo() {}\n\n/* comment */;\n;\nbar() {}\n}", + options: ["always"], + errors: [alwaysError] } ] }); From 0cc88f8de538b1062c1ed2ed0e415207295a7db3 Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Wed, 11 Dec 2019 21:13:54 +0900 Subject: [PATCH 5/7] check token in padding, check end loc of before padding. --- lib/rules/lines-between-class-members.js | 16 +++++++++++++--- tests/lib/rules/lines-between-class-members.js | 5 +++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/lib/rules/lines-between-class-members.js b/lib/rules/lines-between-class-members.js index 20c005d8eea..b45f3949410 100644 --- a/lib/rules/lines-between-class-members.js +++ b/lib/rules/lines-between-class-members.js @@ -85,6 +85,16 @@ module.exports = { return nextFirstToken; } + /** + * Checks if there is a token or comment between two tokens. + * @param {*} before The token before. + * @param {*} after The token after. + * @returns {boolean} True if there is a token or comment between two tokens. + */ + function hasTokenOrCommentBeteen(before, after) { + return sourceCode.getTokensBetween(before, after, { includeComments: true }).length !== 0; + } + return { ClassBody(node) { const body = node.body; @@ -97,8 +107,8 @@ module.exports = { const skip = !isMulti && options[1].exceptAfterSingleLine; const beforePadding = findLastConsecutiveTokenAfter(curLast, nextFirst, 1); const afterPadding = findFirstConsecutiveTokenBefore(nextFirst, curLast, 1); - const isPadded = afterPadding.loc.start.line - beforePadding.loc.start.line > 1; - const hasPaddedComments = sourceCode.commentsExistBetween(beforePadding, afterPadding); + const isPadded = afterPadding.loc.start.line - beforePadding.loc.end.line > 1; + const hasTokenInPadding = hasTokenOrCommentBeteen(beforePadding, afterPadding); const curLineLastToken = findLastConsecutiveTokenAfter(curLast, nextFirst, 0); if ((options[0] === "always" && !skip && !isPadded) || @@ -107,7 +117,7 @@ module.exports = { node: body[i + 1], messageId: isPadded ? "never" : "always", fix(fixer) { - if (hasPaddedComments) { + if (hasTokenInPadding) { return null; } return isPadded diff --git a/tests/lib/rules/lines-between-class-members.js b/tests/lib/rules/lines-between-class-members.js index 2430483aaa6..de98a991114 100644 --- a/tests/lib/rules/lines-between-class-members.js +++ b/tests/lib/rules/lines-between-class-members.js @@ -111,6 +111,11 @@ ruleTester.run("lines-between-class-members", rule, { output: null, options: ["never"], errors: [neverError] + }, { + code: "class foo{ bar(){}\n/* comment-1 */\n\n;\n\n/* comment-3 */\nbaz(){}}", + output: null, + options: ["never"], + errors: [neverError] }, { code: "class A {\nfoo() {}// comment */;\n;\n/* comment */\nbar() {}\n}", output: "class A {\nfoo() {}// comment */;\n\n;\n/* comment */\nbar() {}\n}", From 05a1bfa344cf60e60ba966cfe9046c57de0f6e6c Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Wed, 11 Dec 2019 23:24:18 +0900 Subject: [PATCH 6/7] add jsdoc param type, fix typo --- lib/rules/lines-between-class-members.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/rules/lines-between-class-members.js b/lib/rules/lines-between-class-members.js index b45f3949410..97235303699 100644 --- a/lib/rules/lines-between-class-members.js +++ b/lib/rules/lines-between-class-members.js @@ -87,11 +87,11 @@ module.exports = { /** * Checks if there is a token or comment between two tokens. - * @param {*} before The token before. - * @param {*} after The token after. + * @param {Token} before The token before. + * @param {Token} after The token after. * @returns {boolean} True if there is a token or comment between two tokens. */ - function hasTokenOrCommentBeteen(before, after) { + function hasTokenOrCommentBetween(before, after) { return sourceCode.getTokensBetween(before, after, { includeComments: true }).length !== 0; } @@ -108,7 +108,7 @@ module.exports = { const beforePadding = findLastConsecutiveTokenAfter(curLast, nextFirst, 1); const afterPadding = findFirstConsecutiveTokenBefore(nextFirst, curLast, 1); const isPadded = afterPadding.loc.start.line - beforePadding.loc.end.line > 1; - const hasTokenInPadding = hasTokenOrCommentBeteen(beforePadding, afterPadding); + const hasTokenInPadding = hasTokenOrCommentBetween(beforePadding, afterPadding); const curLineLastToken = findLastConsecutiveTokenAfter(curLast, nextFirst, 0); if ((options[0] === "always" && !skip && !isPadded) || From 6360080b191dfe812d325b851902f825597c19c6 Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Tue, 17 Dec 2019 21:53:43 +0900 Subject: [PATCH 7/7] fix typo in test --- tests/lib/rules/lines-between-class-members.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/lib/rules/lines-between-class-members.js b/tests/lib/rules/lines-between-class-members.js index de98a991114..e4b1c0c092f 100644 --- a/tests/lib/rules/lines-between-class-members.js +++ b/tests/lib/rules/lines-between-class-members.js @@ -41,7 +41,7 @@ ruleTester.run("lines-between-class-members", rule, { "class A{ foo() {}/* a */ \n\n /* b */bar() {}}", "class A {\nfoo() {}\n/* comment */;\n;\n\nbar() {}\n}", - "class A {\nfoo() {}\n// comment */\n\n;\n;\nbar() {}\n}", + "class A {\nfoo() {}\n// comment\n\n;\n;\nbar() {}\n}", "class foo{ bar(){}\n\n;;baz(){}}", "class foo{ bar(){};\n\nbaz(){}}", @@ -102,7 +102,7 @@ ruleTester.run("lines-between-class-members", rule, { options: ["never"], errors: [neverError] }, { - code: "class foo{ bar(){}\n\n// comment */\n\nbaz(){}}", + code: "class foo{ bar(){}\n\n// comment\n\nbaz(){}}", output: null, options: ["never"], errors: [neverError] @@ -117,8 +117,8 @@ ruleTester.run("lines-between-class-members", rule, { options: ["never"], errors: [neverError] }, { - code: "class A {\nfoo() {}// comment */;\n;\n/* comment */\nbar() {}\n}", - output: "class A {\nfoo() {}// comment */;\n\n;\n/* comment */\nbar() {}\n}", + code: "class A {\nfoo() {}// comment\n;\n/* comment */\nbar() {}\n}", + output: "class A {\nfoo() {}// comment\n\n;\n/* comment */\nbar() {}\n}", options: ["always"], errors: [alwaysError] }, {