Indentation fix for multi-line call expression

[saschanaz]

(Related to #1888)

I don't know how to name them, but this fixes a remaining indenting problem with array/function/object literals. This problem is not covered by #3001.

Promise
    .resolve()
    .then(() => {
        "";/**/
    });

// A newline inserted at the marker should get 8-space indentation
// while the indenter currently gives it only 4 spaces.

[DanielRosenwasser]

Hey @saschanaz, thanks for the PR - could you take a look at this as well @vladima?

[DanielRosenwasser]

getContainingList already covers all these cases. Perhaps the logic should be moved into there.

[saschanaz]

You're right, I'll think about this more.

[saschanaz]

I tried some generalization.

[vladima]

argument type should really be a union of PropertyAccessExpression | CallExpression | ElementAccessExpression. Now it works by chance because all these types have similarly named property expression.

[vladima]

@saschanaz Hey, sorry for the delay, thanks for reporting this issue and submitting the fix proposal! The idea of the fix (take actual indentation if expression part of CallExpression is composite and spread across multiple lines) looks like a right direction to go, there are just a few items that should be addressed before the fix should be considered complete - I've left them as comments.

[saschanaz]

It seems I need some help to make a decision, as my latest commit breaks consistenceOnIndentionsOfChainedFunctionCalls test.

var ig: ig;
ig.module(
   'mything'
).requires(
   'otherstuff'
).defines(/*0*//*1*/
});

Inserting newline on 1 gives the line an indentation and so this becomes:

var ig: ig;
ig.module(
   'mything'
).requires(
   'otherstuff'
   ).defines(/*0*/ // (indented)
        /*1*/ // Problem: Test expects 4 spaces here but now there are 8.
});

It now gets 8-space indentation because the previous line already have 4 spaces. Should I just fix the test value and move on? Post it as a separate issue? Or should I fix the indentation?

PS: Oh, now there is a post for this. #3436

[saschanaz]

Let's just fix the test as there is no other opinion.

[DanielRosenwasser]

Sorry for the delay @saschanaz, sometimes it's easy for us to lose track. @vladima has been especially busy on certain critical bugs, but I'll see if I can discuss it with him tomorrow.

[mhegazy]

@saschanaz sorry for the delay. @vladima is the best one to review this change. he has been heads down on some perf and memory issues and he should be back online early next week. sorry again for the delay.

[vladima]

wrap loop body in curlies and add a check that Node.kind is equal to PropertyAccess / ElementAccess / CallExpression

[vladima]

add tests for that will touch all codepaths

[mhegazy]

Thanks @saschanaz! and sorry for the long delay. between release-1.5 stabilization and issues, and release-1.6 planning and feature work thing just fell through. hopefully future PRs would not have to wait that long. thanks again.

[vladima]

Thanks @saschanaz !

[saschanaz]

Great! Should I PR some more changes as @vladima commented?

[mhegazy]

I took care of the comments as I manually merged it.