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

Update: autofix bug in lines-between-class-members (fixes #12391) #12632

Merged
merged 7 commits into from Dec 20, 2019
Merged

Update: autofix bug in lines-between-class-members (fixes #12391) #12632

merged 7 commits into from Dec 20, 2019

Conversation

@yeonjuan
Copy link
Member

@yeonjuan yeonjuan commented Dec 2, 2019

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

This PR will fix three bugs in lines-between-class-members.

  1. #12391

  2. doesn't work well with semicolons.

    • the actual master version.
    /* eslint lines-between-class-members: ["error", "always"] */
    
    class A {
      foo() {}
      /* comment */;
      ;  // No error, but here is no blank line.
      bar() {}
    }
    • this PR
    /* eslint lines-between-class-members: ["error", "always"] */
    
    class A {
      foo() {}
      /* comment */; // Lint Error
      ; 
      bar() {}
    }
  3. incorrect fixing trailing comments

    /* eslint lines-between-class-members: ["error", "always"] */
    
    class foo{ 
      bar(){} //comment
      baz(){}
    }
    • the actual master version
    /* eslint lines-between-class-members: ["error", "always"] */
    
     class foo{ 
       bar(){}
      //comment
       baz(){}
     }
    • this PR
    /* eslint lines-between-class-members: ["error", "always"] */
    
     class foo{ 
       bar(){} //comment
    
       baz(){}
     }

Is there anything you'd like reviewers to focus on?
#12391

@yeonjuan yeonjuan changed the title Fix: comments deletion bug in lines-between-class-members(fixes #12391) Fix: autofix bug in lines-between-class-members(fixes #12391) Dec 5, 2019
@yeonjuan yeonjuan changed the title Fix: autofix bug in lines-between-class-members(fixes #12391) Fix: autofix bug in lines-between-class-members (fixes #12391) Dec 5, 2019
Copy link
Member

@kaicataldo kaicataldo left a comment

LGTM. Thanks for working on this! I'd like at least one more pair of eyes on this before we merge.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

This is a very nice code but I think it can still remove comments in some cases, in particular when there are blank lines both before and after the comment?

/* eslint lines-between-class-members:["error", "never"] */

class MyClass {
    myMethod1 (param) {
    }
    
    /** 
     * this comment will be removed?
     */
    
    myMethod2 (param) {
    }
}

Wondering if we should anyway merge this because it does fix the original issue (and these should be the most common scenarios), or maybe implement something similar to how padding-line-between-statements handles comments.

Another solution might be to ignore class members that have comments between them and defer to the lines-around-comment rule. It seems that these two rules can have some unavoidable conflicts anyway.

@yeonjuan
Copy link
Member Author

@yeonjuan yeonjuan commented Dec 8, 2019

@mdjermanovic
oh..Yes. this is how padding-line-between-statements handles comments - demo(no fix).
I changed it. thanks! :)

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Dec 9, 2019

Thank for the change! Looks like the comments are completely safe now.

There is another thing to note. In some extremely rare cases, this fix can change the behavior of this rule to report more or fewer warnings. The actual master version looks for comments before the next member (using sourceCode.getCommentsBefore, which returns comments after the first preceding non-comment token), while this fix searches from both sides.

This will almost always produce the same result at the end, but a class body can also contain useless empty members (semicolons), which don't exist in AST.

An example where the actual version doesn't report warning, but the version from this PR does:

/* eslint lines-between-class-members: ["error", "always"] */

class A {
  foo() {}
  /* comment */;
  bar() {}
}

This rule in general at the moment doesn't work well with these semicolons. I think it's okay to merge this PR as is (just mark with Update because it can produce more warnings) and maybe later analyze and fix handling of semicolons in a separate PR.

@yeonjuan yeonjuan changed the title Fix: autofix bug in lines-between-class-members (fixes #12391) Update: autofix bug in lines-between-class-members (fixes #12391) Dec 9, 2019
@yeonjuan
Copy link
Member Author

@yeonjuan yeonjuan commented Dec 9, 2019

@mdjermanovic
Thanks for the elaborate review. I changed the pr message(fix -> update).

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Dec 9, 2019

TIL! Nice catch, @mdjermanovic 👍

I'm personally not sure we should defer the fix until later. Would it be possible to check for semicolon tokens in addition to comments in this PR?

@yeonjuan
Copy link
Member Author

@yeonjuan yeonjuan commented Dec 10, 2019

@kaicataldo @mdjermanovic

May I ask some questions?
Before I move on, I would like to double-check whether I understand correctly. :)

The actual version doesn't report an error in case1. But I assume that this is a bug. ...right?

  • case1
/* eslint lines-between-class-members: ["error", "always"] */

class A {
  foo() {}
  /* comment */; // No error, but here is no blank line.
  bar() {}
}

The PR version report an error in case1, but no error in case2.

  • case2
/* eslint lines-between-class-members: ["error", "always"] */

class A {
  foo() {}
  /* comment */;
  ;  // No error, but here is no blank line.
  bar() {}
}

So I need to add check for semicolon tokens. Do I get this right?

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Dec 10, 2019

The actual version doesn't report an error in case1. But I assume that this is a bug. ...right?

In my opinion - yes, this is a bug, It seems that the actual master version doesn't handle semicolons well. In some cases, this rule can even remove them:

/* eslint lines-between-class-members: ["error", "never"] */

class A {
  foo() {}
  ;
  bar() {}
}

This is fixed to:

/* eslint lines-between-class-members: ["error", "never"] */

class A {
  foo() {}
bar() {}
}

This doesn't look like something this rule should do, no-extra-semi removes these semicolons.

So I need to add check for semicolon tokens. Do I get this right?

While these semicolons are technically class members (I think), this rule should probably treat them as it treats comments - don't require/disallow empty lines around semicolons, but a line with a semicolon shouldn't be seen as an empty line.

Maybe getLastConsecutiveTokenAfter/getFirstConsecutiveTokenBefore shouldn't explicitly check for comments and semicolons, but use first/last token of the following/previous class members as a stop token instead.

@yeonjuan
Copy link
Member Author

@yeonjuan yeonjuan commented Dec 10, 2019

@mdjermanovic @kaicataldo
there is one more bug(in my opinion) in the actual version. - demo

/* eslint lines-between-class-members: ["error", "always"] */

class foo{ 
  bar(){} //comment
  baz(){}
}

This is fixed to:

/* eslint lines-between-class-members: ["error", "always"] */

class foo{ 
  bar(){}

  //comment
  baz(){}
}

It's because the actual version inserts newline only after the last member node without considering trailing comments or semicolon.
I pushed the commit(7ec30b2) includes fixing this problem also. thanks.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Thanks for all the changes!

This PR now fixes 3 different bugs: #12391, semicolons, and "always" autofix.

I left two notes for some minor edge cases.

lib/rules/lines-between-class-members.js Outdated Show resolved Hide resolved
lib/rules/lines-between-class-members.js Outdated Show resolved Hide resolved
@yeonjuan
Copy link
Member Author

@yeonjuan yeonjuan commented Dec 11, 2019

@mdjermanovic
thanks for the reviews. I'm all done :)

@yeonjuan yeonjuan requested a review from mdjermanovic Dec 14, 2019
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Looks good! Just to remove some probably extra chars in the tests, to avoid confusion about what was the intention of these tests.

tests/lib/rules/lines-between-class-members.js Outdated Show resolved Hide resolved
tests/lib/rules/lines-between-class-members.js Outdated Show resolved Hide resolved
tests/lib/rules/lines-between-class-members.js Outdated Show resolved Hide resolved
@yeonjuan
Copy link
Member Author

@yeonjuan yeonjuan commented Dec 17, 2019

@mdjermanovic
Oh.. sorry for my mistake. I fixed typo.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

@kaicataldo kaicataldo merged commit 5c25a26 into eslint:master Dec 20, 2019
11 of 12 checks passed
11 of 12 checks passed
@github-actions
Verify Files
Details
@github-actions
Test (ubuntu-latest, 13.x)
Details
@github-actions
Test (ubuntu-latest, 12.x)
Details
@github-actions
Test (ubuntu-latest, 10.x)
Details
@github-actions
Test (ubuntu-latest, 8.x)
Details
@github-actions
Test (ubuntu-latest, 8.10.0)
Details
@github-actions
Test (windows-latest, 12.x)
Details
@github-actions
Test (macOS-latest, 12.x)
Details
@github-actions
Browser Test Browser Test
Details
@eslint-deprecated
commit-message PR title follows commit message guidelines
Details
licence/cla Contributor License Agreement is signed.
Details
@eslint-deprecated
release-monitor No patch release is pending
Details
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Dec 20, 2019

Thanks for contributing!

@yeonjuan yeonjuan deleted the yeonjuan:line-between-class-member branch Jan 3, 2020
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jun 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants