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

Odd behavior with Extra Comma #363

Closed
goatslacker opened this Issue Nov 27, 2011 · 5 comments

Comments

Projects
None yet
3 participants
@goatslacker
Copy link
Member

goatslacker commented Nov 27, 2011

When white is true

/*jshint white: true */
var f = [1,];

JSHint will tell me I'm missing a space after the comma and that there is an extra comma. It should just tell me there's an extra comma.

And the following is even stranger:

var a = { a: 1,, b: 2};

Outputs

othertest.js: line 1, col 16, Missing space after ','.
othertest.js: line 1, col 16, Extra comma.
othertest.js: line 1, col 16, Expected '}' to match '{' from line 1 and instead saw ','.
othertest.js: line 1, col 17, Missing semicolon.
othertest.js: line 1, col 21, Label 'b' on 2 statement.
othertest.js: line 1, col 21, Expected '2' to have an indentation at 1 instead at 21.
othertest.js: line 1, col 21, Expected an assignment or function call and instead saw an expression.
othertest.js: line 1, col 22, Missing semicolon.
othertest.js: line 1, col 22, Expected '(end)' and instead saw '}'.

I just need to know there's an extra comma.

@antris

This comment has been minimized.

Copy link

antris commented Jan 17, 2012

These commits at least hide the confusing whitespace warnings from the beginning of the reports.

Note that I do not know very well how the JSHint parser works and if this is a proper way to do the check, but at least my fix does not break any make tests. Someone more knowledgeable must double check this code before merging.

@goatslacker

This comment has been minimized.

Copy link
Member

goatslacker commented Jan 18, 2012

Cool, thanks. Will review this tonight/tomorrow.

@goatslacker

This comment has been minimized.

Copy link
Member

goatslacker commented Jan 19, 2012

So correct me if I'm wrong but I'm thinking if white is true

var a = [1,,2];

Should tell me there's an extra comma and a space missing. That would rule out f45ebff

Adding && nexttoken.id !== '}' to L2038 would also treat cases like { a: 1, b: 2, }

The only issue now is when an object literal has two, or more, commas in a row.

@antris

This comment has been minimized.

Copy link

antris commented Jan 19, 2012

Reporting var a = [1,,2]; is a bit trickier than I thought at first. In that case a would be an array, where a[0] === 1, a[1] === undefined and a[2] === 2. That means [1, , 2] would be a valid (although very unreadable) way on initializing an array with these specific characteristics.

There's many options to approach this:

  1. You could assume that it's a typo and the developer meant to write [1, 2], raising an extra comma error and just one whitespace warning.
  2. You could raise two whitespace warnings but no extra comma warning, suggesting the developer to turn [1,,2] into [1, , 2].
  3. Raise the whitespace warnings and in addition suggest that instead of using [1, , 2] notation, undefined would be explicitly written: [1, undefined, 2].

Any others? I'm not sure if I like any of those. They all make assumptions about what the developer is trying to do. Though, I hate the third options least, because if the developer has made a typo about the extra comma, it would also show itself as a logical error, because a[1] would be undefined instead of 2. Then the developer could during debugging go back to see how it is initialized and deduce oneself that there is an extra comma. The developer definitely knows best whether it's a typo or the third case despite what JSHint says.

@valueof

This comment has been minimized.

Copy link
Member

valueof commented Aug 20, 2012

Not worth fixing in the current tree and already fixed in Next.

@valueof valueof closed this Aug 20, 2012

@valueof valueof reopened this Oct 30, 2012

@valueof valueof closed this in 632985f Dec 5, 2012

jugglinmike added a commit to jugglinmike/jshint that referenced this issue Oct 21, 2014

Don't call nonadjacent function for trailing commas.
This patch prevents the false positive `Missing whitespace after...`
from happening on trailing commas. I implemented a temporary solution
because the better one, the one I'd like to have, requires quite an
overhaul.

Closes jshintGH-363.

danielctull pushed a commit to danielctull-forks/jshint that referenced this issue Jan 14, 2018

Don't use 'self' in module loader shims
It is browser-specific, and people might also want to
load the primitive way outside of browser.

Closes jshint#363
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment