Skip to content

Fix: Check indentation on multiline variable declarations (fixes #6911)#6912

Closed
Trott wants to merge 1 commit intoeslint:masterfrom
Trott:indent-string-continuation
Closed

Fix: Check indentation on multiline variable declarations (fixes #6911)#6912
Trott wants to merge 1 commit intoeslint:masterfrom
Trott:indent-string-continuation

Conversation

@Trott
Copy link
Copy Markdown
Contributor

@Trott Trott commented Aug 15, 2016

What issue does this pull request address?
#6911: Indent rule doesn't cover multiline VariableDeclarations

What changes did you make? (Give an overview)
Added a BinaryExpression option to the indent rule that (for now) only is used inside a VariableDeclaration. It warns if continuations are not indented.

Is there anything you'd like reviewers to focus on?

Add BinaryExpression option for indent rule to check multi-line
variable declarations for indentation issues.

…nt#6911)

Add `BinaryExpression` option for `indent` rule to check multi-line
variable declarations for indentation issues.
@eslintbot
Copy link
Copy Markdown

LGTM

Comment thread docs/rules/indent.md
* Indent of 2 spaces with `MemberExpression` set to `0` will indent the multi-line property chains with 0 spaces.
* Indent of 2 spaces with `MemberExpression` set to `1` will indent the multi-line property chains with 2 spaces.
* Indent of 2 spaces with `MemberExpression` set to `2` will indent the multi-line property chains with 4 spaces.
* Indent of 2 spaces with `BinaryExpression` set to `0` will indent the multi-line continuations with 0 spaces.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the use-case for indenting with user-defined number of indentation levels? Shouldn't we always indent by just 1 indentation level?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two reasons:

  • Doing it that way to be consistent with existing options (MemberExpression, SwitchCase, etc.)
  • Turns out there are folks who indent string continuations in variable declarations at different levels. Node.js project uses two for everything else, but four for this situation, apparently. See test: add uncaught exception test for debugger nodejs/node#8087 (comment)
    ¯_(ツ)_/¯

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. OK, that sounds like a good enough reason:-)

@mysticatea mysticatea added the do not merge This pull request should not be merged yet label Aug 17, 2016
@mysticatea
Copy link
Copy Markdown
Member

mysticatea commented Aug 17, 2016

I added "do not merge" label since the issue has not been accepted.

Added a BinaryExpression option to the indent rule that (for now) only is used inside a VariableDeclaration. It warns if continuations are not indented.

If we want to warn all BinaryExpressions in future, how do we do? Maybe it's going to a breaking change?

@Trott
Copy link
Copy Markdown
Contributor Author

Trott commented Aug 17, 2016

If we want to warn all BinaryExpressions in future, how do we do? Maybe it's going to a breaking change?

Quite possibly. Each expansion of BinaryExpression in the future would need to be evaluated.

@Trott Trott closed this Sep 26, 2016
@eslint-deprecated eslint-deprecated Bot locked and limited conversation to collaborators Feb 6, 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 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

archived due to age This issue has been archived; please open a new issue for any further discussion do not merge This pull request should not be merged yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants