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

Lint rule: no-confusing-subscript-indentation #92

Open
wcjohnson opened this issue Sep 27, 2018 · 3 comments
Open

Lint rule: no-confusing-subscript-indentation #92

wcjohnson opened this issue Sep 27, 2018 · 3 comments

Comments

@wcjohnson
Copy link
Owner

We should point out the difference between

a
.b
.c()

and

a
.b
[c]
.d()

so that someone doesn't step on a land mine by inserting a bracket into a non-indented subscript chain.

We can suggest that the user either indent to subscript, add a newline for a separate expr, or put the bracket on the same line with its parent.

Since this rule is preventing a possible error it should be enabled by default.

@wcjohnson wcjohnson changed the title Lint rule: Ambiguous bracket in multiline subscript chain Lint rule: no-confusing-subscript-indentation Sep 27, 2018
@rattrayalex
Copy link

I think if we want to do something other than #93, I would recommend enforcing that the opening square bracket of a member expression is on the same line as the object, like this:

// illegal
a
  [b]

// legal (1)
a[b]

// legal (2)
a[
  b
]

I'm not sure whether to autofix to (1) or (2), but we should autofix to one of them.

Maybe something like, if it's at the end of a member expression chain, autofix to (1); if it's followed by a non-computed member expression (ie; a dot), autofix to (2). Probably don't want to get too clever though; ultimately we'll want prettier to make these decisions.

@rattrayalex
Copy link

rattrayalex commented Sep 28, 2018

Actually, poking around with prettier, it seems to have pretty good behavior here, choosing either something like

foo.d.d
  .baz[b]
  .d.a.q;

or

foo.d.baz[
  bdd
].d.a.q;

either way keeping the [ on the same line as the preceding part of the chain.

We could autofix to just (1) for now and recommend (2) in the docs for when it's needed.

Either way, I think a linter along these lines would really help this situation (though I still think we should include #93 anyway)

@wcjohnson
Copy link
Owner Author

The general plan would be to include all three rules in the linter, and let the user pick between them -- just as ESLint core bundles a whole bunch of style rules, most of which are disabled by default.

This one would be enabled by default, as it is the least strict of all the rules and it prevents a potentially serious error.

Unfortunately the fixing capabilities of eslint are not on par with those of prettier (it can't regenerate the output using the AST) and the transformation has to look like a series of text insertions before/after nodes. Something like "delete newline before bracket, insert newline and two spaces after bracket, insert newline after computed property node" which would make output like:

a
.b
[c]

goes to

a
.b[
  c
]

Although upon reflection, this may not be a case where autofixing is appropriate. For example, someone might write

object
.method()
[variable] = value

with the actual intent of the assignment being separate from the subscript, and then the autofix will have changed the semantics of their code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants