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

Enforce indentation in member expressions #628

Closed
feross opened this issue Sep 16, 2016 · 9 comments
Closed

Enforce indentation in member expressions #628

feross opened this issue Sep 16, 2016 · 9 comments

Comments

@feross
Copy link
Member

feross commented Sep 16, 2016

I propose adding { "MemberExpression": 1 } to the indent option.

Example of incorrect code:

foo
.bar
.baz()

foo
    .bar
  .baz()

Examples of correct code:

foo
  .bar
  .baz()

// Any indentation is permitted in variable declarations and assignments.
var bip = aardvark.badger
                  .coyote

A small change, but adds more consistency.

Rule: http://eslint.org/docs/rules/indent#memberexpression

@feross
Copy link
Member Author

feross commented Sep 16, 2016

Smallish ecosystem impact:

# tests 422
# pass  407
# fail  15

Also, automatically fixable.

@feross feross modified the milestone: standard v9 Sep 16, 2016
@timoxley
Copy link
Contributor

timoxley commented Sep 18, 2016

Rule looks good, with the exception of the concern below:

Not totally keen on forcing chaining to be indented e.g.

stream
  .pipe(through(() => {

  })) //  having closing braces indented like this looks broken IMO
another.line() // especially as there's no closing brace on this line

Having parens/braces dangling out there looks like there's something unclosed.

Would prefer having consistently open indentation closed by a paren or brace, as it allows you to quickly find indentation/brace issues. Currently it's immediately obvious when something may be amiss with your code if there's a closing paren and the wrong amount of indentation around it, even if there's no syntax error.

    })
  }
}
// ahhh. rest easy now son, we've closed all the braces

Not strongly against this though since it could just be because I wasn't using any editor-integrated linter until recently, so there was much more reliance on easily parsable visual clues in the code. Perhaps now I can relax this and rely more on standard to find such problems.

@dcousens
Copy link
Member

@timoxley that is a great point, and is very disconcerting/confusing when reading code with that disjoined indentation due to this chaining indentation.

@feross feross modified the milestones: standard v9, standard v10 Feb 9, 2017
@glukki
Copy link

glukki commented Mar 1, 2017

Please, set it to some value!

Different code editors have different default settings for chain indentation when you run their code reformat tool. Lines jump left-right without any rule, and git history gets messed (like in my current project 😢 ).

@feross feross modified the milestones: standard v10, standard v11 Mar 2, 2017
kemitchell added a commit to commonform/commonform.org that referenced this issue Apr 27, 2017
A rule to this effect is coming in JavaScript Standard Style:
standard/standard#628
@timoxley
Copy link
Contributor

timoxley commented Jan 22, 2018

@feross I think it's time to bring this up again since the eslint default in standard v11 seems to have swung to requiring member expression indentation and it's not clear a consensus was reached for standard going either way regarding this.

@timoxley
Copy link
Contributor

I stand by my earlier argument that indenting members creates a weird closing paren/curly indentation unlike any other syntax and thus is undesirable.

i.e. changing whether something is chained or not shouldn't need to change the entire indentation of all arguments and where the closing paren is:

Consider wanting to break this chain onto multiple lines:

a.pipe().pipe().then(() => {
  // function body
})
b()

Without member indentation, closing curly stays in the same position, as does the function body:

a
.pipe()
.pipe()
.then(() => {
  // function body
})
b()

With member indentation, everything chained now needs to be indented for some reason:

a
  .pipe()
  .pipe()
  .then(() => {
    // function body also needs indentation now?
  }) // closing paren suddenly indented?
b() // no other rule results in a closing curly/paren + newline changing -2 levels of indentation

@bcomnes
Copy link
Member

bcomnes commented Feb 13, 2018

No indentations does seem a tad more pragmatic

@feross
Copy link
Member Author

feross commented Feb 18, 2018

I see the arguments for both ways of doing it. My thoughts are that whenever something that is conceptually one single unit goes onto multiple lines, you want some way of distinguishing the part that has extended onto later lines, so you can recognize it, even without the context of the former lines. Indentation is a good way to distinguish these lines, IMO.

I do something similar for argument lists:

function foo (
  argument1, argument2, argument3
) {
  // function body...
}

And for really long console.log lines:

console.log(
  variable1,
  variable2,
  variable3,
  variable4
)

And for math:

const x = (variable1 * variable2 * variable3) + variable4 -
  (variable5 / variable6)

And for ternaries:

const bool = condition
  ? variable1
  : variable2

And in JSX:

const App = () => {
  return (
    <div
      class='my-cool-class-name'
      style='some: cool; styles: here;'
      aria-label='accessibility stuff'
    >
      Text goes here
    </div>
  )
}

I think all these cases are more confusing without the extra indent:

function foo (
argument1, argument2, argument3
) {
  // function body...
}

console.log(
variable1,
variable2,
variable3,
variable4
)

const x = (variable1 * variable2 * variable3) + variable4 -
(variable5 / variable6)

const bool = condition
? variable1
: variable2

const App = () => {
  return (
    <div
    class='my-cool-class-name'
    style='some: cool; styles: here;'
    aria-label='accessibility stuff'
    >
      Text goes here
    </div>
  )
}

Given these cases (which if they're not enforced yet, I'd like to eventually enforce) I think it's more consistent to indent in this case as well.

@feross feross modified the milestones: standard v12, standard v11 Feb 18, 2018
@feross
Copy link
Member Author

feross commented Feb 19, 2018

🗣 Standard 11 is released! Run npm install standard@latest --save-dev to update to the latest version. This will also update the version in package.json

Changelog: https://github.com/standard/standard/blob/master/CHANGELOG.md#1100---2018-02-18

@feross feross closed this as completed Feb 19, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

No branches or pull requests

6 participants
@timoxley @feross @glukki @bcomnes @dcousens and others