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

[keyword-spacing] override this before: false with greater than #14712

Closed
sam-s4s opened this issue Jun 16, 2021 · 3 comments · Fixed by #15172
Closed

[keyword-spacing] override this before: false with greater than #14712

sam-s4s opened this issue Jun 16, 2021 · 3 comments · Fixed by #15172
Assignees
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 repro:yes rule Relates to ESLint's core rules
Projects

Comments

@sam-s4s
Copy link

sam-s4s commented Jun 16, 2021

Tell us about your environment

  • ESLint Version: 7.27.0
  • Node Version: 14.17.0

What parser (default, @babel/eslint-parser, @typescript-eslint/parser, etc.) are you using?
@typescript-eslint/parser
but also tested with only using eslint and the problem still exists.

Please show your full configuration:

Configuration
{
  "rules": {
    'keyword-spacing': 'off',
    '@typescript-eslint/keyword-spacing': [
        'error',
        {
            "overrides": {
                "this": { "before": null },
                "new": { "before": null }
            }
        }
    ],
  }
}

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

Here is a demo showing the problem:
https://eslint.org/demo#eyJ0ZXh0IjoiLyogZXNsaW50IGtleXdvcmQtc3BhY2luZzogWydlcnJvcicsIHtcbiAgICAgICAgICAgIFwib3ZlcnJpZGVzXCI6IHtcbiAgICAgICAgICAgICAgICBcInRoaXNcIjogeyBcImJlZm9yZVwiOiBmYWxzZSB9LFxuICAgICAgICAgICAgICAgIFwibmV3XCI6IHsgXCJiZWZvcmVcIjogZmFsc2UgfVxuICAgICAgICAgICAgfVxuICAgICAgICB9XSAqL1xuXG5pZiAodGhpcy5yZWZ1bmRBbW91bnQgPiB0aGlzLm1heGltdW1SZWZ1bmQpIHt9Iiwib3B0aW9ucyI6eyJwYXJzZXJPcHRpb25zIjp7ImVjbWFWZXJzaW9uIjoxMSwic291cmNlVHlwZSI6InNjcmlwdCIsImVjbWFGZWF0dXJlcyI6e319LCJydWxlcyI6e30sImVudiI6e319fQ==

(note that when you change the > into a < the lint error goes away.

if (this.refundAmount > this.maximumRefund) { // the same thing with a less than works fine

I wasn't sure what to expect from this configuration of the rule. I had to set "this" and "new" to before: false, because I don't want a space between casting angle brackets <Thing>this.blah or <Thing>new Blah().
And I was happy when it seemed to work in almost all cases. Obviously I'm using this.blah in thousands of places, very commonly with spaces in front, and almost all of them work. The only exception is the "greater than" symbol, used for comparisons. Less than (and everything else) seems to work fine.

So given it only gives an error with the exact combination > this I feel like it's a bug.

What did you expect to happen?
No errors

What actually happened? Please copy-paste the actual, raw output from ESLint.
283:41 error Unexpected space(s) before "this" @typescript-eslint/keyword-spacing

Steps to reproduce this issue:

  1. (see demo above)
@sam-s4s sam-s4s added bug ESLint is working incorrectly repro:needed labels Jun 16, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Jun 16, 2021
@aladdin-add
Copy link
Member

aladdin-add commented Jun 16, 2021

looks like a bug to me! are you willing to open a PR?

@aladdin-add aladdin-add added repro:yes rule Relates to ESLint's core rules and removed repro:needed labels Jun 16, 2021
@aladdin-add aladdin-add moved this from Needs Triage to Ready to Implement in Triage Jun 16, 2021
@sam-s4s
Copy link
Author

sam-s4s commented Jun 16, 2021

I am unfamiliar with the lint code and internal process, so not sure if I'd be a good candidate. But it is possible, if that's the best option :)

Also the more I think about it, the more I wonder if this should be treated like a keyword at all. It's really just used like a variable, and this rule does not cover those. eg...

let blah = { moo: 7 };
if (blah.moo) {}
if (this.moo) {} // why should this be treated any differently to the above line?

(possibly new also, aside from the fact that you always want a space after it)

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Oct 15, 2021
@mdjermanovic
Copy link
Member

mdjermanovic commented Oct 15, 2021

Marked as accepted since it conflicts with the space-infix-ops rule:

/* eslint keyword-spacing: ['error', {
    "overrides": {
        "this": { "before": false }
    }
}]*/

/* eslint space-infix-ops: error */

if (this.refundAmount > this.maximumRefund) {} // keyword-spacing error

if (this.refundAmount >this.maximumRefund) {} // space-infix-ops error

Online Demo

I'll work on this.

@mdjermanovic mdjermanovic self-assigned this Oct 15, 2021
@mdjermanovic mdjermanovic moved this from Ready to Implement to Pull Request Opened in Triage Oct 16, 2021
Triage automation moved this from Pull Request Opened to Complete Oct 22, 2021
mdjermanovic added a commit that referenced this issue Oct 22, 2021
…) (#15172)

* Add regression tests

* Add passing tests

* Add more passing tests

* Add failing tests

* Fix the issue
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Apr 21, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 21, 2022
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 repro:yes rule Relates to ESLint's core rules
Projects
Triage
Complete
Development

Successfully merging a pull request may close this issue.

3 participants