From b9c64585095a8b40474b1a361da65305cb1c91f9 Mon Sep 17 00:00:00 2001 From: Ivan Kopeykin Date: Wed, 31 Jul 2019 11:42:54 +0300 Subject: [PATCH] Fix false positives for multi-line url() in max-line-length (#4169) --- lib/rules/max-line-length/__tests__/index.js | 93 ++++++++++++++++---- lib/rules/max-line-length/index.js | 60 +++++++++---- 2 files changed, 119 insertions(+), 34 deletions(-) diff --git a/lib/rules/max-line-length/__tests__/index.js b/lib/rules/max-line-length/__tests__/index.js index 88de796354..cb5573dfbe 100644 --- a/lib/rules/max-line-length/__tests__/index.js +++ b/lib/rules/max-line-length/__tests__/index.js @@ -2,6 +2,8 @@ const rule = require(".."); const { messages, ruleName } = rule; +const testUrl = "somethingsomething something\tsomething"; +const _21whitespaces = new Array(21).fill("\u0020").join(""); testRule(rule, { ruleName, @@ -21,25 +23,25 @@ testRule(rule, { code: "@media print {\n a {\n color: pink;\n }\n}" }, { - code: "a {\n background: url(somethingsomethingsomethingsomething);\n}" + code: `a {\n background: url("${testUrl}");\n}` }, { - code: "a {\n background: uRl(somethingsomethingsomethingsomething);\n}" + code: `a {\n background: uRl("${testUrl}");\n}` }, { - code: "a {\n background: URL(somethingsomethingsomethingsomething);\n}" + code: `a {\n background: URL("${testUrl}");\n}` }, { - code: - "a {\n background: url(\n somethingsomethingsomethingsomething\n );\n}" + code: `a {\n background: url(\n "${testUrl}"\n );\n}` }, { - code: - "a {\n background: uRl(\n somethingsomethingsomethingsomething\n );\n}" + code: `a {\n background: uRl(\n "${testUrl}"\n );\n}` }, { - code: - "a {\n background: URL(\n somethingsomethingsomethingsomething\n );\n}" + code: `a {\n background: URL(\n "${testUrl}"\n );\n}` + }, + { + code: `a {\n background: URL(\n "${testUrl}"\n );\nbackground: url(\n "${testUrl}"\n);\n}` }, { code: "a { margin: 0 2px; }\r\n" @@ -48,13 +50,32 @@ testRule(rule, { code: "a { margin: 0 2px; }\r\na { margin: 4px 0; }\n" }, { - code: '@import url("somethingsomethingsomethingsomething.css") print;' + code: `@import url("${testUrl}") print;` + }, + { + code: `@import '${testUrl}';` }, { - code: "@import 'somethingsomethingsomethingsomething.css';" + code: `@import "${testUrl}";` }, { code: "@import 'svg-something' projection;" + }, + { + // exactly 20 whitespaces + code: + "a {\n background-image:\nurl(\n" + + _21whitespaces.substr(0, 20) + + `"${testUrl}"` + + "\n); }" + }, + { + // exactly 20 whitespaces + code: + "a {\n background-image:\nurl(\n" + + `"${testUrl}"` + + _21whitespaces.substr(0, 20) + + "\n); }" } ], @@ -83,6 +104,12 @@ testRule(rule, { line: 3, column: 33 }, + { + code: `a {\n background: URL(\n "${testUrl}"\n );\n background: url(\n "${testUrl}"\n);\n}`, + message: messages.expected(20), + line: 5, + column: 27 + }, { code: "@media print {\n a {\n color: pink; background: orange;\n }\n}", @@ -103,18 +130,22 @@ testRule(rule, { column: 21 }, { - code: - '@import url("somethingsomethingsomethingsomething.css") projection, tv;', + code: `@import url("${testUrl}") projection, tv;`, message: messages.expected(20), line: 1, - column: 71 + column: 69 }, { - code: - "@import 'somethingsomethingsomethingsomething.css' projection, tv;", + code: `@import '${testUrl}' projection, tv;`, + message: messages.expected(20), + line: 1, + column: 64 + }, + { + code: `@import "${testUrl}" projection, tv;`, message: messages.expected(20), line: 1, - column: 66 + column: 64 }, { code: "@import 'svg-something' screens, tv;", @@ -123,10 +154,10 @@ testRule(rule, { column: 48 }, { - code: "a { background-image: url('some ignored url'); }", + code: `a { background-image: url("${testUrl}"); }`, message: messages.expected(20), line: 1, - column: 48 + column: 70 }, { code: @@ -134,6 +165,30 @@ testRule(rule, { message: messages.expected(20), line: 2, column: 272 + }, + { + // more than 20 whitespaces + code: `a { + background-image: + url( + ${_21whitespaces}"${testUrl}" + ); + }`, + message: messages.expected(20), + line: 4, + column: 69 + }, + { + // more than 20 whitespaces + code: `a { + background-image: + url( + "${testUrl}"${_21whitespaces} + ); + }`, + message: messages.expected(20), + line: 4, + column: 69 } ] }); diff --git a/lib/rules/max-line-length/index.js b/lib/rules/max-line-length/index.js index 366a259780..9ead30c153 100644 --- a/lib/rules/max-line-length/index.js +++ b/lib/rules/max-line-length/index.js @@ -9,6 +9,10 @@ const styleSearch = require("style-search"); const validateOptions = require("../../utils/validateOptions"); const ruleName = "max-line-length"; +const EXCLUDED_PATTERNS = [ + /url\(\s*((.|[\t\v])*[^\s])\s*\)/gi, // allow tab, whitespace in url content + /@import\s+(['"](.|[\t\v])*['"])/gi +]; const messages = ruleMessages(ruleName, { expected: max => @@ -43,6 +47,21 @@ const rule = function(maxLength, options, context) { const ignoreNonComments = optionsMatches(options, "ignore", "non-comments"); const ignoreComments = optionsMatches(options, "ignore", "comments"); const rootString = context.fix ? root.toString() : root.source.input.css; + // Array of skipped sub strings, i.e `url(...)`, `@import "..."` + const skippedSubStrings = []; + let skippedSubStringsIndex = 0; + + EXCLUDED_PATTERNS.forEach(pattern => + execall(pattern, rootString).forEach(match => { + const startOfSubString = + match.index + match.match.indexOf(_.get(match, "subMatches[0]", "")); + + return skippedSubStrings.push([ + startOfSubString, + startOfSubString + _.get(match, "subMatches[0].length", 0) + ]); + }) + ); // Check first line checkNewline(rootString, { endIndex: 0 }, root); @@ -62,6 +81,28 @@ const rule = function(maxLength, options, context) { }); } + function tryToPopSubString(start, end) { + const [startSubString, endSubString] = skippedSubStrings[ + skippedSubStringsIndex + ]; + + // Excluded substring does not presented in current line + if (end < startSubString) { + return 0; + } + + // Compute excluded substring size regarding to current line indexes + const excluded = + Math.min(end, endSubString) - Math.max(start, startSubString); + + // Current substring is out of range for next lines + if (endSubString <= end) { + skippedSubStringsIndex++; + } + + return excluded; + } + function checkNewline(rootString, match, root) { let nextNewlineIndex = rootString.indexOf("\n", match.endIndex); @@ -75,6 +116,9 @@ const rule = function(maxLength, options, context) { } const rawLineLength = nextNewlineIndex - match.endIndex; + const excludedLength = skippedSubStrings[skippedSubStringsIndex] + ? tryToPopSubString(match.endIndex, nextNewlineIndex) + : 0; const lineText = rootString.slice(match.endIndex, nextNewlineIndex); // Case sensitive ignorePattern match @@ -82,25 +126,11 @@ const rule = function(maxLength, options, context) { return; } - const urlArgumentsLength = execall(/url\((.*)\)/gi, lineText).reduce( - (result, match) => { - return result + _.get(match, "subMatches[0].length", 0); - }, - 0 - ); - - const importUrlsLength = execall( - /@import\s+(['"].*['"])/gi, - lineText - ).reduce((result, match) => { - return result + _.get(match, "subMatches[0].length", 0); - }, 0); - // If the line's length is less than or equal to the specified // max, ignore it ... So anything below is liable to be complained about. // **Note that the length of any url arguments or import urls // are excluded from the calculation.** - if (rawLineLength - urlArgumentsLength - importUrlsLength <= maxLength) { + if (rawLineLength - excludedLength <= maxLength) { return; }