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

Comment before comma trigger comma-spacing error #2408

Closed
tiansh opened this issue Apr 29, 2015 · 5 comments
Closed

Comment before comma trigger comma-spacing error #2408

tiansh opened this issue Apr 29, 2015 · 5 comments
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

@tiansh
Copy link

tiansh commented Apr 29, 2015

Use default setting on http://eslint.org/demo/

Codes:

var myfunc = function (x, y, z) {
  "use strict";
  /* ... */
  return [x, y, z];
};
myfunc(404, true/* bla bla bla */, "hello");

Result:

6:33 - There should be no space before ','.

i'm new here, not sure if this is a feature or bug. maybe related to #1457

@gyandeeps gyandeeps added the triage An ESLint team member will look at this issue soon label Apr 29, 2015
@gyandeeps
Copy link
Member

I don't think this is related to 1457, since that was different.
But the code right now for comma-spacing doesn't account for the scenario you described, which is having a comment.

@nzakas
Copy link
Member

nzakas commented Apr 29, 2015

Related to #2211.

@nzakas nzakas added bug ESLint is working incorrectly rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Apr 29, 2015
@gcochard
Copy link
Contributor

gcochard commented May 8, 2015

Is this also related to #1659?

@nzakas
Copy link
Member

nzakas commented May 8, 2015

Slightly, but not directly.

@silverwind
Copy link
Contributor

Also been hit by this with code such as this:

[
  [0x61, 0x7A] /*a-z*/,
  [0x41, 0x5A] /*A-Z*/,
  0x2E /*'.'*/, 0x2B /*'+'*/, 0x2D /*'-'*/
]

Removing the space before the comment doesn't seem to help. I think context.getTokensBetween needs to be updated to include comments.

silverwind added a commit to nodejs/node that referenced this issue May 11, 2015
Certain cases with comments inside arrays or object literals fail to
pass eslint's comma-spacing rule. This change sets the comma-spacing
rule to the 'warn' level.

Once eslint/eslint#2408 is resolved and
released, this rule should be set back to 'error' level.

PR-URL: #1672
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
nzakas added a commit that referenced this issue May 15, 2015
Fix: Allow comment before comma for comma-spacing rule (fixes #2408)
Fishrock123 pushed a commit to Fishrock123/node that referenced this issue May 19, 2015
Certain cases with comments inside arrays or object literals fail to
pass eslint's comma-spacing rule. This change sets the comma-spacing
rule to the 'warn' level.

Once eslint/eslint#2408 is resolved and
released, this rule should be set back to 'error' level.

PR-URL: nodejs#1672
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
silverwind added a commit to silverwind/node that referenced this issue Jun 28, 2015
The rule was disabled because of an eslint bug which is now resolved.
All code in lib was already conforming and only test code needed a few
changes to make the linter happy with this rule enabled.

Ref: eslint/eslint#2408
silverwind added a commit to nodejs/node that referenced this issue Jun 29, 2015
The rule was disabled because of an eslint bug which is now resolved.
All code in lib was already conforming and only test code needed a few
changes to make the linter happy with this rule enabled.

Ref: eslint/eslint#2408

PR-URL: #2072
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
Reviewed-by: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alex Kocharin <alex@kocharin.ru>
mscdex pushed a commit to mscdex/io.js that referenced this issue Jul 9, 2015
The rule was disabled because of an eslint bug which is now resolved.
All code in lib was already conforming and only test code needed a few
changes to make the linter happy with this rule enabled.

Ref: eslint/eslint#2408

PR-URL: nodejs#2072
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
Reviewed-by: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alex Kocharin <alex@kocharin.ru>
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 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 7, 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
Development

No branches or pull requests

5 participants