Fix: lines-between-class-members (fixes #9665)

[sakabar]

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)
I fixed the bug of issue #9665

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

[jsf-clabot]

All committers have signed the CLA.

[mysticatea]

Thank you for the contribution!

Direction looks good to me, but I have some concerns. Could you add some tests?

class A {
    foo() {} // a comment

    bar() {}
}

class A {
    foo() {}
    /* a */ /* b */

    bar() {}
}

class A {
    foo() {}

    /* a */ bar() {}
}

And could you sign our CLA?

[sakabar]

@mysticatea Thank you for your review.
I signed CLA. Is this OK?

I added those test cases, which failed. I'm debugging my code now.

[aladdin-add]

I see. a blank line was wrongly expected before /*foo*/ since the rule assumes the comment is for the following member bar -- this is not always true.

class Foo{
foo(){} 
/* foo*/

bar(){}
}

An easy fix is to also check lines after the comment /*foo*/.

[mysticatea]

Hm, #9680 (comment) shows "not signed yet", but licence/cla check is passed.

@ilyavolodin do you have idea?

[sakabar]

@mysticatea Sorry, I did strange things.

1. I committed my code with an old e-mail address.
2. jsf-clabot worked : not signed yet
3. I noticed that I used the old e-mail address.
4. I did git reset, git commit, and git push -f
5. I signed the CLA with a new e-mail address
6. licence/cla passed. However, the message of jsf-clabot didn't be edited.

[mysticatea]

@sakabar Thank you for the information!

Looks like a bug of @jsf-clabot.

[sakabar]

@aladdin-add

An easy fix is to also check lines after the comment /*foo*/.

Sorry, I can't imagine code when there are several comments. I implemented this code in my way.

[aladdin-add]

the function seems a little complicated for me, could you add a comment to say what was happening, thanks!

[sakabar]

@aladdin-add Sure, I'll add comments.

[sakabar]

@mysticatea
I added the first and second test case in your review. The third has already been added in lines-between-class-members.js#L37.

All checks have passed. I'll add more comments because my code is complicated.

[aladdin-add]

LGTM. thanks!!

[mysticatea]

LGTM, thank you!

[sakabar]

@aladdin-add @mysticatea
Thank you for reviewing. Should I squash these commits?

[aladdin-add]

@sakabar not a necessary, we will squash the commits when merging. 😄

[sakabar]

@aladdin-add Sure!

[platinumazure]

This LGTM so far. Are there any invalid tests that could/should be added, though? It would be especially good to capture any new (or clarified) autofix behavior in some new tests.

[mysticatea]

@platinumazure I think additional invalid tests are unnecessary because this PR is a bug fix of false positive.

[aladdin-add]

thanks for contributing! @sakabar

[sakabar]

@mysticatea @aladdin-add Thank you for your reviewing!