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

False positive on rule lines-between-class-memebers if a comment occurs between class members #9665

Closed
jrencz opened this issue Nov 29, 2017 · 5 comments · Fixed by mono-js/mono-notifications#5 or mono-js/mono-push#5 · May be fixed by ali8889/emerald-wallet#4, DmytroSkrypnyk/test_bootstrap#6 or ali8889/emerald-wallet#17
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@jrencz
Copy link
Contributor

jrencz commented Nov 29, 2017

Tell us about your environment

  • ESLint Version: 4.12
  • Node Version: 8.9.1
  • npm Version: 5.5.1

What parser (default, Babel-ESLint, etc.) are you using?
n/a

Please show your full configuration:

Configuration
{
    "parserOptions": {
        "ecmaVersion": 6,
        "sourceType": "script",
        "ecmaFeatures": {}
    },
    "rules": {
        "lines-between-class-members": 2
    },
    "env": {}
}

(generated by ESLint Demo)

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

I have a class which purpose is to be extended by other classes in order to provide actual implementation of some methods.
The class has placeholder methods that only throw, but don't use context (the subclass implementation will)

Since this is an exception i wrapped the method with /* eslint-disable class-methods-use-this */ just before the doc block and /* eslint-enable class-methods-use-this */ just after the method body like that:

class Foo {
  /* eslint-disable class-methods-use-this */
  /**
   * @returns {Array}
   */
  getFoo() {
    throw new Error('Child class should reimplement this method');
  }
  /* eslint-enable class-methods-use-this */

  /**
   * @returns {undefined}
   */
  doSomething() {}
}

Demo showing false positive (demo1)

Demo reduced to bare minimum showing false positive (demo2). This one proves that any block comment between class members break the rule

What's interesting: Moving the /* eslint-enable class-methods-use-this */ from line 9 to line 10 (so that the padding line is above the /* eslint-enable */ directive) solves the problem (another demo with no error: demo1-no-error, and demo2-no-error)

What did you expect to happen?
I think the rule should not report an error on line 14, especially since there's a blank line 10 which logically meets rule requirements.
Placement of /* eslint-* */ directives should not affect whether the rule reports error or not if there's one empty line between members.

What actually happened? Please include the actual, raw output from ESLint.
Rule reports error in line 14, column 3 (see demo1 and demo2).

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Nov 29, 2017
@aladdin-add aladdin-add added bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Dec 1, 2017
@sakabar
Copy link
Contributor

sakabar commented Dec 3, 2017

I'm trying on this!

sakabar pushed a commit to sakabar/eslint that referenced this issue Dec 3, 2017
sakabar pushed a commit to sakabar/eslint that referenced this issue Dec 3, 2017
@sakabar
Copy link
Contributor

sakabar commented Dec 3, 2017

I made PR. #9680
Cound you review it?

@sakabar
Copy link
Contributor

sakabar commented Dec 3, 2017

My commit message doesn't follow guidelines. I'll fix it.

sakabar pushed a commit to sakabar/eslint that referenced this issue Dec 3, 2017
`lines-between-class-memebers` if a comment occurs between class members
sakabar added a commit to sakabar/eslint that referenced this issue Dec 3, 2017
`lines-between-class-memebers` if a comment occurs between class members
@sakabar
Copy link
Contributor

sakabar commented Dec 3, 2017

@aladdin-add Hi. Could you review my PR (#9680) ? All checks have passed.

@aladdin-add
Copy link
Member

thanks! I can reproduce it.

@aladdin-add aladdin-add added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 3, 2017
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jun 14, 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 Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
3 participants