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-statements-per-line false positive (fixes #6173, fixes #6153) #6192

Merged
merged 1 commit into from
May 17, 2016

Conversation

mysticatea
Copy link
Member

Fixes #6173,
Fixes #6153.

I rewrote the traversal logic of this rule.
Before, it uses a for loop in BlockStatement handler.
After this PR, it uses ESLint's traversing. So this rule gets addressing nested nodes correctly.

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @vitorbal, @glenjamin and @alberto to be potential reviewers

@glenjamin
Copy link
Contributor

I'm not sure about the name "dangling host type", but this looks much more solid than the old way.

I was surprised how little traversal there was in here when I came to do the last patch!

@glenjamin
Copy link
Contributor

Maybe "single child allowed" - naming based on the usage rather than the category

@mysticatea mysticatea force-pushed the max-statements-per-line/fix-traversal branch from 7f3306a to 0621850 Compare May 17, 2016 13:34
@eslintbot
Copy link

LGTM

@mysticatea
Copy link
Member Author

Thank you, @glenjamin. I updated the variable name.

I try to explain more since this PR is a large change.
This rule is depending on traversal order. It's checking whether a statement exists in the same line as the previous statement. But it was using wrong order.


a; if (b) { c; d; }
y;
z;

In this case, before, the order is aif (b)yzcd. Then, y is not the same line as if (b). c is not the same line as z. It had wrong order for nested statements. So the rule will show unintentional result on this code.

After this PR, the order is aif (b)cdyz. This addresses nested nodes correctly.


if (a)
    b;
else
    c;

In this case, the order is if (a)c in both before and after.
Before, it was comparing between the first line of a statement and the last line of the previous statement, for all cases. So the rule will show unintentional result on this code because the last line of IfStatement and the first line of c; is the same (this is #6173).

After this PR, it will use the first line of statements for nested statements. So in this case, it will compare between the first lines of the IfStatement and c;.

@nzakas
Copy link
Member

nzakas commented May 17, 2016

Lgtm

@nzakas nzakas merged commit 72335eb into master May 17, 2016
@mysticatea mysticatea deleted the max-statements-per-line/fix-traversal branch May 17, 2016 21:31
islemaster added a commit to islemaster/p5.play that referenced this pull request May 19, 2016
A regression in ESLint causes false positives from the max-statements-per-line rule.  So far versions 2.10.0-2.10.2 are affected. See eslint/eslint#6173

The fix (eslint/eslint#6192) has been merged and seems likely to land in 2.10.3.  Once that happens we should update our depedency to allow >= the fixed version.
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants