fmt creates less-than-ideal indentation

[ahakanbaba]

Hi,
I ran into several cases where the auto formatter does not display the code in a reasonable ways.

Here are some examples.:

// 1)
{
    objects():: [object]
                + if condition then [object2]
    else [],
}

// 2)
{
    objects():: [object]
                + if condition then
        [object2]
    else [],
}

// 3)
{
    objects():: [object] + if condition then
        [object2]
    else [],
}

I am using the the "-i -n 4 " options to fmt via vim-jsonnet.
Can be seen here
https://github.com/google/vim-jsonnet/blob/master/autoload/jsonnet.vim#L83

It would be great to improve these.

[ahakanbaba]

I have one more example that involve function calls.

local someValue = someObject.someFunc(
                                      param1=value1,
                                      param2=[
    value2,
    value3,
]
).someFunc();

[ahakanbaba]

I also want to revisit the enforced alignment of parameters to a function with the open parenthesis "("

Example

local someValue = someObject.someFunc(
                                      param1=value1,

What is the reaction to allow another type of alignment?
For example:

local someValue = someObject.someFunc(
    param1=value1,
    param2=value2);

Instead of getting aligned with the "(" we prefer to be indented from the start of the statement.

What do you think ?

[ahakanbaba]

We really like the automated formatting idea and would like to use it more extensively.
Any response to this issue would be greatly appreciated @sparkprime

I can spend some free time on these issues if you could give some pointers. Maybe a similar previous PR, a test that exercises the relevant path ?

@huggsboson @ghodss

[sparkprime]

I've been thinking about this, but I haven't had time to dive into the code yet. The examples you've given do look bad and everything you're asking for is reasonable. I recall the logic involved (formatter.cpp FixIndentation class) is fairly complex.

The case of

local someValue = someObject.someFunc(
                                      param1=value1,

should definitely not be happening because it should only line up if the first param is on the same line, otherwise it should indent by only 4 (or whatever you set).

[sparkprime]

So I verified the function parameters problem, not sure why it's doing it but I think it will be an easy fix since it's designed to do what you want, it's probably just a typo or something.

Your original case (3) is questionable because if you have it do this:

{
    objects():: [object] + if condition then
                        [object2]
                else [],
}

Then this pattern would also be indented and I don't think we want that:

{
    someField: lib.Class + {
        ...
    }
}

So unless we have a special behavior for if (as opposed to { }) then I don't we can change that.

The case (2) is behaving the same as (3). We could try to make it understand the fact that the if is on a new line and indent it to the beginning of the new line in that case. This would also fix (1) and in those two cases you would get:

// 1)
{
    objects():: [object]
                + if condition then [object2]
                else [],
}

// 2)
{
    objects():: [object]
                + if condition then
                    [object2]
                else [],
}

[sparkprime]

Ok the function parameters problem is fixed in #290. The others need more thinking.

[ahakanbaba]

Thanks looks good.
Please let me know if there is anything I can help with about the rest of the cases.

[sparkprime]

How do you feel about

// 1)
{
    objects():: [object]
                + if condition then [object2]
                else [],
}

// 2)
{
    objects():: [object]
                + if condition then
                    [object2]
                else [],
}

// 3)
{
    objects():: [object] + if condition then
        [object2]
    else [],
}

[huggsboson]

I prefer: [ object, object2 if condition, ] :) On Jan 24, 2017 4:39 PM, "Dave Cunningham" <[email protected]> wrote: How do you feel about // 1) { objects():: [object] + if condition then [object2] else [], } // 2) { objects():: [object] + if condition then [object2] else [], } // 3) { objects():: [object] + if condition then [object2] else [], } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#278 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADUzzHedMAakdsk1eE7jUxbtcY3ZVsTqks5rVpnLgaJpZM4LcWxN> .

[sparkprime]

Uh huh :)

Seriously, if addition of that feature would make a more significant difference then it could be prioritized...

[ahakanbaba]

How do you feel about

// 1)
{
objects():: [object]
+ if condition then [object2]
else [],
}

// 2)
{
objects():: [object]
+ if condition then
[object2]
else [],
}

// 3)
{
objects():: [object] + if condition then
[object2]
else [],
}

These all look good to me. Thanks for your attention.

We have a quite large jsonnet code base now with all the weird combinations of arrays, objects, conditions, etc.
I would like to get a pre-release and try to tun fmt on all the files and see how they look.

With that experiment I may know more about the style improvements.

[huggsboson]

I don't really love any of the three. If there was an inline conditional like we've discussed in #234 I think a lot of these cases would go away. @ahakanbaba we may want to look into adding something to util that filters nulls out of an array so we can avoid a lot of this optional concatenation into an array (and thereby mess up ordering of elements).

[ahakanbaba]

I feel like the discussion is diverging here a little bit.

We are talking about indentation of the currently existing conditional.

If you do not like indentations in #278 (comment) do you have better suggestions?

Even tough there is a plan for a syntax like

lhs = value_for_true if condition else value_for_else. 

The indentations for the current if conditions should be fixed.