Update: Check indentation for multi-line chained properties (refs #1801) #5940

Trott · 2016-04-23T06:01:42Z

The end user can choose whether and how much to indent.

foo()
.bar
.baz()

vs.

foo()
  .bar
  .baz()

and so forth...

eslintbot · 2016-04-23T06:01:43Z

LGTM

jquerybot · 2016-04-23T06:01:45Z

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.

If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

Trott · 2016-04-23T06:21:07Z

I've signed the CLA and the information provided appears to match my git author information. Not sure if there's something up with the automatic CLA checker thingy or if I'm just Doing It Wrong somehow...

Trott · 2016-04-23T06:56:46Z

There's going to definitely be people that will prefer this:

var a = foo.bar()
           .baz()
           .bip()
           .bap();

...instead of:

var a = foo.bar()
  .baz()
  .bip()
  .bap();

It may be possible to write a rule smart enough to distinguish between that and other cases where it may make sense to indent-by-two rather than align, but I suspect there will be many edge cases. Indentation of then() in promise chains would seem to be a likely one too.

Maybe I'm overthinking it?

Trott · 2016-04-23T20:29:52Z

(Rebased against master, conflicts resolved, force pushed.)

Trott · 2016-04-23T20:50:28Z

Looks like the Travis failure was a bitbucket issue?

gyandeeps · 2016-04-23T22:38:38Z

@Trott Thanks a lot. I think implementation looks good. CC: @BYK
Couple of minor things:

Please change your commit message to start with "Update:"
Since this rule is able to apply auto-fixes, the invalid cases should have an output property with fixed code.
Would like to see some tests with tabs (/t) in it as well as some with chaining value greater than 1.

Trott · 2016-04-24T04:49:10Z

@gyandeeps Thanks! I think I've addressed each of your requests at this point.

One question:

Should there be a setting to disable the indentation check on chained properties and/or a setting to ignore the indentation but to guarantee alignment? The idea is to enable people using the indent rule to have code like this if they wish:

var a = foo.bar()
           .baz()
           .bip()
           .bap();

BYK · 2016-04-24T06:20:33Z

I actually have some concerns:

The name, Chaining is not aligned with our other option names. I think it should just say MemberExpression.
Giving a numerical indent multiplier was a big mistake for other options so I think following that mistake is not also great. I feel like this should be a string value: indent, none, align or { indent: "loose", align: true} to separate indentation from aligning (indent can be any of these: "strict", "loose", "none"). This would allow code like the following

_.map(something, somethingElse)
 .filter(lalala)

Trott · 2016-04-25T04:05:53Z

If you can set it to align rather than indent, is that an indication that perhaps it should be its own separate rule rather than part of the indent rule?

BYK · 2016-05-02T12:59:11Z

@Trott - disagree. This is clearly related to indentation. Aligning from the left-most side is always indent-related.

nzakas · 2016-05-18T17:37:18Z

@BYK @gyandeeps what's the next step here?

BYK · 2016-05-19T22:08:02Z

I haven't changed my position and @Trott seems to be fed up with my insistence :D

Trott · 2016-05-19T22:52:44Z

@BYK Nah, not fed up. Just fell of my radar...

eslintbot · 2016-05-19T22:58:13Z

LGTM

Trott · 2016-05-19T22:58:43Z

Rebased against master, resolved the resulting conflict in the documentation file, force pushed.

eslintbot · 2016-05-19T23:08:16Z

LGTM

Trott · 2016-05-19T23:08:42Z

Chaining -> MemberExpression per @BYK. Force-pushed.

nzakas · 2016-06-03T16:38:20Z

The plan is make the other options follow the same, better new option scheme (with backwards compatibility) if you agree. Reason is they still suffer from the same problem (think about VariableDeclaration).

This seems like a very confusing incremental step towards that. If that's your goal, then I think we should start with a new issue describing all the changes you want to make. I don't like the idea of using this task as a backdoor for significant changes to this rule.

BYK · 2016-06-06T10:14:08Z

@nzakas so is your preference to make this like the others and then file a separate ticket to make a transition for all (in a backwards-compatible manner)?

If so I can live with that.

nzakas · 2016-06-06T16:05:55Z

@BYK yup, exactly.

BYK · 2016-06-06T16:13:51Z

Alright, removing my reservations about how the option is coded for this PR then. I'll file a new ticket once this is completed.

nzakas · 2016-06-06T19:18:19Z

@BYK so is this ready to merge?

BYK · 2016-06-16T13:31:09Z

lib/rules/indent.js

+
+                var dot = context.getTokenBefore(node.property);
+
+                if (dot.type === "Punctuator" && dot.value === ".") {


This logic here is a bit confusing to me. Why are we checking the dot additionally? Here's what I think:

If the dot is on the previous line, we only need to check the MemberExpression node.

If the dot is on the same line with MemberExpression, then we only need to check the dot's indentation.

Am I right?

Everywhere else, the checking for whether the node is the first item on the line is delegated to the check* functions, so I was following that pattern. They all just send everything to check*() and let that function figure out if it's the first thing on the line and needs to be checked or not.

Your suggested logic would work just fine too. If that would be less confusing, I can do that instead.

BYK · 2016-06-16T13:34:03Z

Sorry for the late review. I have a few concerns around how the Punctuator dot is handled and reported. Rest looks good (especially tests).

Trott · 2016-06-28T04:32:22Z

@BYK I've responded to the concerns/questions you raised inline. Do those responses satisfy your concerns? If not, I'm happy to keep working at this...

kaicataldo · 2016-07-06T02:52:08Z

@BYK Friendly ping

@Trott Looks like we might have had a merge conflict crop up since your last update

eslintbot · 2016-07-13T22:07:40Z

LGTM

Trott · 2016-07-13T22:08:38Z

@kaicataldo Merge conflict resolved.

@kaicataldo @BYK Let me know if there's anything I can do to get this un-stalled.

kaicataldo · 2016-07-14T03:50:53Z

@Trott Thanks!

@eslint/eslint-team Friendly ping to make sure this doesn't get lost

nzakas · 2016-07-18T18:08:03Z

@Trott can you rebase?

tarwich · 2016-07-18T19:07:12Z

😭 So close!
I've got all these programmers writing

foo()
  .bar()
    .bin();

I can't wait to enforce

foo()
.bar()
.bin();

GL @Trott I'm cheering for you over here.

…nt#1801) The end user can choose whether and how much to indent.

eslintbot · 2016-07-18T23:34:28Z

LGTM

Trott · 2016-07-18T23:34:44Z

@Trott can you rebase?

@nzakas Done!

nzakas · 2016-07-20T18:04:51Z

LGTM. I think the open questions are minor ones, so it seems like we can go with this and then see what feedback we get. Thanks @Trott for sticking with this, I know it's been a long journey and we really appreciate your patience.

curiousdannii · 2016-07-29T22:27:54Z

I don't fully understand the technicalities of semver, but adding this made our lint fail. And I can't fix it by simply setting MemberExpression to a different default because I sometimes indent these lines and sometimes don't. (Well I could fix it with inline rules, but that's a nasty hack.)

This should've been a new rule, or delayed to sermver major. 👎

platinumazure · 2016-07-29T22:49:23Z

@curiousdannii Please read our README for information on our semver policy. The short version is, we will sometimes release CI-breaking changes under semver-minor if they are new features for a rule (with opt-in configuration) or if they are bugfixes that will result in more errors being reported. We may have erred in this case, but I want to make sure it is understood that CI-breaking changes are not considered semver-major in all cases here.

If you want to avoid CI breaks, you can always use tilde ranges in your dependency configuration.

If you have questions/concerns, please stop by our Gitter chat. Thanks!

curiousdannii · 2016-07-29T22:58:49Z

@platinumazure I did think that that would probably be the case.

But there should still be a way to disable the MemberExpression check.

platinumazure · 2016-07-29T23:01:12Z

Absolutely. We do have an issue and multiple proposed PRs available, so
we'll consider the best way forward probably this weekend or early next
week. In the meantime, you can pin your ESLint dev dependency while we get
this sorted. Thanks so much for your patience!

On Jul 29, 2016 3:58 PM, "Dannii Willis" notifications@github.com wrote:

@platinumazure https://github.com/platinumazure I did think that that
would probably be the case.

But there should still be a way to disable the MemberExpression check.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5940 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARWejR3D9cxKT4gOPIXSxZrNTi-NF2tks5qaoWqgaJpZM4IOHqz
.

jquerybot added the CLA: Error label Apr 23, 2016

Trott mentioned this pull request Apr 23, 2016

tools: lint for function argument alignment nodejs/node#6268

Closed

2 tasks

jquerybot added CLA: Valid and removed CLA: Error labels Apr 23, 2016

Trott force-pushed the indent-chain branch from 789bd04 to b4c61bb Compare April 23, 2016 20:29

Trott force-pushed the indent-chain branch 3 times, most recently from 21fe90b to 8017738 Compare April 24, 2016 03:04

Trott changed the title ~~New: Validate indentation for multi-line chained properties (refs #1801)~~ Update: Check indentation for multi-line chained properties (refs #1801) Apr 24, 2016

Trott force-pushed the indent-chain branch from 8017738 to 11e42a3 Compare April 24, 2016 03:22

Trott force-pushed the indent-chain branch from 11e42a3 to 8ccfc78 Compare May 19, 2016 22:58

Trott force-pushed the indent-chain branch from 8ccfc78 to 7a43a0d Compare May 19, 2016 23:08

BYK reviewed Jun 16, 2016
View reviewed changes

Trott force-pushed the indent-chain branch from 7a43a0d to 0d3b561 Compare July 13, 2016 22:07

Update: Check indentation of multi-line chained properties (refs esli…

bc6446a

…nt#1801) The end user can choose whether and how much to indent.

Trott force-pushed the indent-chain branch from 0d3b561 to bc6446a Compare July 18, 2016 23:34

nzakas merged commit 95ea25a into eslint:master Jul 20, 2016

curiousdannii mentioned this pull request Jul 29, 2016

eslint@3.2.0 breaks build 🚨 alerque/stack-verse-mapper#31

Closed

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

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


		var dot = context.getTokenBefore(node.property);

		if (dot.type === "Punctuator" && dot.value === ".") {

Update: Check indentation for multi-line chained properties (refs #1801) #5940

Update: Check indentation for multi-line chained properties (refs #1801) #5940

Conversation

Trott commented Apr 23, 2016

eslintbot commented Apr 23, 2016

jquerybot commented Apr 23, 2016

Trott commented Apr 23, 2016

Trott commented Apr 23, 2016

Trott commented Apr 23, 2016

Trott commented Apr 23, 2016

gyandeeps commented Apr 23, 2016

Trott commented Apr 24, 2016

BYK commented Apr 24, 2016 • edited

Trott commented Apr 25, 2016

BYK commented May 2, 2016 • edited

nzakas commented May 18, 2016

BYK commented May 19, 2016

Trott commented May 19, 2016

eslintbot commented May 19, 2016

Trott commented May 19, 2016

eslintbot commented May 19, 2016

Trott commented May 19, 2016

nzakas commented Jun 3, 2016

BYK commented Jun 6, 2016

nzakas commented Jun 6, 2016

BYK commented Jun 6, 2016

nzakas commented Jun 6, 2016

BYK Jun 16, 2016

Choose a reason for hiding this comment

Trott Jun 20, 2016

Choose a reason for hiding this comment

BYK commented Jun 16, 2016

Trott commented Jun 28, 2016

kaicataldo commented Jul 6, 2016 • edited

eslintbot commented Jul 13, 2016

Trott commented Jul 13, 2016

kaicataldo commented Jul 14, 2016

nzakas commented Jul 18, 2016

tarwich commented Jul 18, 2016 • edited by IanVS

eslintbot commented Jul 18, 2016

Trott commented Jul 18, 2016

nzakas commented Jul 20, 2016

curiousdannii commented Jul 29, 2016 • edited

platinumazure commented Jul 29, 2016

curiousdannii commented Jul 29, 2016

platinumazure commented Jul 29, 2016

BYK commented Apr 24, 2016 •

edited

BYK commented May 2, 2016 •

edited

kaicataldo commented Jul 6, 2016 •

edited

tarwich commented Jul 18, 2016 •

edited by IanVS

curiousdannii commented Jul 29, 2016 •

edited