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

New rule: don't use destructing for index access #551

Closed
LinusU opened this issue Jun 20, 2016 · 7 comments

Comments

@LinusU
Copy link
Member

commented Jun 20, 2016

Today I saw some code that looked a bit like this (admittedly with less commas, but still)

const [, , , , code] = /(\d)?(test|hello)( - )?(\d)/.exec('...')

While it works, I think it's much more idiomatic to write it like so:

const code = /(\d)?(test|hello)( - )?(\d)/.exec('...')[4]

How about a rule enforcing this?

@dcousens

This comment has been minimized.

Copy link
Member

commented Jun 20, 2016

What about [, , , , , code, tests]

@LinusU

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2016

Good question, you could write it as [code, tests] = _.slice(6). Don't know if it's better or not though. It's certainly easier to read than counting the commas though...

@dcousens

This comment has been minimized.

Copy link
Member

commented Jun 20, 2016

I agree that the initial case is a bit odd, especially when taken to such an extreme, but I don't think its going to be easy to make a hard and fast rule about this that won't seem inconsistent just because n > 1.

@LinusU

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2016

Maybe make the rule only about when extracting a single value? Just throwing out some ideas, any input is highly welcome :)

@timoxley

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2016

While it works, I think it's much more idiomatic to write it like so:

I don't particularly like the comma version either but it's most probably just because I'm not used to that even being an option. Some patterns are only idiomatic because an alternative didn't exist until recently.

@feross

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

This feels too fancy for my taste:

const [, , , , code, tests] = /(\d)?(test|hello)( - )?(\d)/.exec('...')

I prefer dumb, explicit code that won't impress anyone:

const re = /(\d)?(test|hello)( - )?(\d)/.exec('...')
const code = re[4]
const tests = re[5]

From RULES.md:

"Clever short-hands are discouraged, in favor of clear and readable expressions, whenever possible."

That said, I don't think this is an easy thing to enforce (no existing eslint rule for it, and where would you draw the line? How many commas is too many?)

@feross

This comment has been minimized.

Copy link
Member

commented Aug 19, 2016

@LinusU Can you open this as an issue on ESLint to see what the ESLint team thinks? There's nothing we can do in standard until there's a rule in ESLint, so I'm going to close this to keep only actionable issues in the issue tracker.

Once this is part of ESLint, I will notice it in the changelog when I upgrade the ESLint version or you can open an issue/PR to discuss adding the rule to standard.

@feross feross closed this Aug 19, 2016

@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.
4 participants
You can’t perform that action at this time.