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

fix max-line-length rule #4169

Merged
merged 2 commits into from Jul 31, 2019
Merged

Conversation

vankop
Copy link
Member

@vankop vankop commented Jul 24, 2019

Which issue, if any, is this issue related to?

Fixes #4059

Is there anything in the PR that needs further explanation?

Nothing

@hudochenkov
Copy link
Member

So many changes, I don't understand what it does :(

I wonder would this issue be fixed if it was updating RexExp to accept new lines:

- const urlArgumentsLength = execall(/url\((.*)\)/gi, lineText).reduce(
+ const urlArgumentsLength = execall(/url\(([\s\S]*)\)/gi, lineText).reduce(

@vankop
Copy link
Member Author

vankop commented Jul 27, 2019

So many changes, I don't understand what it does :(

I wonder would this issue be fixed if it was updating RexExp to accept new lines:

- const urlArgumentsLength = execall(/url\((.*)\)/gi, lineText).reduce(
+ const urlArgumentsLength = execall(/url\(([\s\S]*)\)/gi, lineText).reduce(

vankop@5677f47
This test case will fall

@vankop
Copy link
Member Author

vankop commented Jul 27, 2019

current code execute regex line-by-line thats why it falls ( url( could be splitted between lines)

const skippedSubStrings = [];
let skippedSubStringsIndex = 0;

EXCLUDED_PATTERNS.forEach(pattern =>
Copy link
Member Author

@vankop vankop Jul 27, 2019

Choose a reason for hiding this comment

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

all excluded patterns create array of excluded indexes, for instance:

url
 (
 "blabla"
)

will return [[7, 15]], where 7 is first " and 15 is next after "blabla"

@@ -75,32 +116,21 @@ const rule = function(maxLength, options, context) {
}

const rawLineLength = nextNewlineIndex - match.endIndex;
const excludedLength = skippedSubStrings[skippedSubStringsIndex]
Copy link
Member Author

@vankop vankop Jul 27, 2019

Choose a reason for hiding this comment

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

compute excluded substring length (for current line), for instance:

url(
\t\t"bla bla"\u0020);

will give excludedLength equals to "bla bla".length on line 2

Copy link
Member Author

@vankop vankop Jul 27, 2019

Choose a reason for hiding this comment

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

it is a bit tricky since

url("bla \
bla \
");

will give excludedLength equals to "bla \\".length on line 1

Math.min(end, endSubString) - Math.max(start, startSubString);

// Current substring is out of range for next lines
if (endSubString <= end) {
Copy link
Member Author

@vankop vankop Jul 27, 2019

Choose a reason for hiding this comment

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

current line last index + 1 is over current excluded range end

];

// Excluded substring does not presented in current line
if (end < startSubString) {
Copy link
Member Author

Choose a reason for hiding this comment

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

current excluded range is not presented on current line

}

// Compute excluded substring size regarding to current line indexes
const excluded =
Copy link
Member Author

Choose a reason for hiding this comment

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

Compute excluded substring size regarding to current line indexes =)

@vankop
Copy link
Member Author

vankop commented Jul 27, 2019

@hudochenkov i have add some explanation

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

LGTM

@hudochenkov hudochenkov merged commit b9c6458 into stylelint:master Jul 31, 2019
@hudochenkov
Copy link
Member

  • Fixed: max-line-length false positives for multi-line url() (#4169).

@vankop vankop deleted the max-line-length-rule-fix branch July 31, 2019 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fix false positives for multi-line url() in max-line-length
3 participants