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

Should not allow space before bracket notation #389

Closed
mattdesl opened this issue Jan 20, 2016 · 7 comments

Comments

@mattdesl
Copy link

commented Jan 20, 2016

I just got bit by a small bug because the linting didn't catch it. Might be worth disallowing this, I can't see any reason it should be legit.

setup('shims argv', 'argv.js', JSON.stringify([
  path.resolve(__dirname, 'fixtures', 'argv.js'),
  '--foobar'
]) [ '--foobar' ])
  ^
  Error is here, missing a comma

It wouldn't catch the same code if there was no space, but at least when a space is there (i.e. in the case of a missing comma) it could be caught early.

Thoughts? 😄

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2016

I agree it looks like a bug. It might be worth filing this on eslint.

@kaicataldo

This comment has been minimized.

Copy link

commented Jan 28, 2016

Wait, are you sure this is incorrect syntax? It might just be because it's late, but I'm not seeing the error...I don't know the particulars of the specific example, but it looks like valid syntax to me:

function1('string', 'string', function2([function3(variable, 'string', 'string'), 'string'])['property of returned value of function 2']);

Edit: Oh, I see what you meant now! Rather than a property of the returned value of function2, that was intended to be an argument of function1. So the answer is that both (comma and no comma) are legit syntax with very different meanings - it doesn't seem to be a linting error :)

@feross

This comment has been minimized.

Copy link
Member

commented Feb 4, 2016

@kaicataldo Even though it's technically valid syntax, I think @mattdesl was hoping standard would report it as error, because property accesses shouldn't have spaces after them. So it's either inconsistent style, or a programmer error.

Happy to enable a rule for this, if it's available in eslint.

@feross

This comment has been minimized.

Copy link
Member

commented Feb 4, 2016

Doesn't look like there's an eslint rule for this.

@feross feross closed this Feb 4, 2016

@feross

This comment has been minimized.

Copy link
Member

commented Feb 4, 2016

@mattdesl Are you interested in opening an issue on eslint to request this rule?

@mattdesl

This comment has been minimized.

Copy link
Author

commented Feb 4, 2016

It is such a minor thing that I can't really be bothered. 😁 Thanks for looking tho.

@feross feross removed the blocked label Feb 4, 2016

@feross

This comment has been minimized.

Copy link
Member

commented Feb 5, 2016

I think we can actually catch this with the http://eslint.org/docs/2.0.0/rules/no-whitespace-before-property rule. Will ship this in standard v6.

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
5 participants
You can’t perform that action at this time.