Collection entry indentation inconsistency in fmt

[ahakanbaba]

Consider the following indentations auto generated by jsonnet fmt

{
    collCorrect1: [{
        name: "Good",
    }],
    collCorrect2: [{
        name: "Bad",
    }, someOther(),
    ],
    collWeird: [{
                    name: "Bad",
                },
                someOther(),
    ],
}

It looks like the someOther() function being on a new line confuses the indentation calculation for the collWeird example.

Would you consider this as a bug @sparkprime ?

[ahakanbaba]

@huggsboson FYI

[sparkprime]

What do you expect it to do? Personally I find collCorrect2 to look the weirdest because there is a } immediately followed by a ] at the same indentation level.

For collWeird, I say that if you choose not to start a new line, you're expecting everything to be lined up. The ] is closing the initial [ so is not subject to that environment created within the [ ].

[sparkprime]

Oh and yeah, in the case of someOther() being on the same line as the comma before it... in that case there is no element of the list with a linebreak preceding it, which is why it has chosen to not indent it (same as collCorrect1, but then there was only 1 element of the list so it is more obvious).

[sparkprime]

It's kinda hard to codify rules like this...

[huggsboson]

I would land on this:

{
    collCorrect2: [
        {
            name: "Bad",
        }, 
        someOther(),
    ]

I've largely started eschewing the [{ and }] on the same line as I keep running into edge cases like this and end up landing on.

[
    {
       ...
    },
    ...
]

Which takes up way more space but at least formats consistently.

[ahakanbaba]

I should have been more clear in the behavior I see as surprising.

I may be wrong but please bear with me.
The collection has two entries separated with a comma.

It looks to me that the second entry's location with respect to the comma affects the indentation of the first entry. That was surprising to me. Please let me know if you think that is expected behavior.

Almost the same example in a more directed way and with line numbers :

25     coll2: [{
26         name: "name",
27     }, someOther(),
28     ],
29     coll3: [{
30                 name: "Bad",
31             },
32             someOther(),
33     ],
34

Personally I find collCorrect2 to look the weirdest because there is a } immediately followed by a ] at the same indentation level.

You may be right. I should not have used "Weird" in the key name. Maybe the coll2 should look like this ?

25     coll2: [{
26                 name: "name",
27             }, someOther(),
28     ],
29

But then, that implementation may break other patterns that I do not know at this point.

I only wanted to point out that changing the line of the second element modifies the indentation of the first element.

It's kinda hard to codify rules like this...

Absolutely, thanks a lot for your help in making jsonnet code more beautiful.

[ahakanbaba]

Oh and yeah, in the case of someOther() being on the same line as the comma before it... in that case there is no element of the list with a linebreak preceding it, which is why it has chosen to not indent it (same as collCorrect1, but then there was only 1 element of the list so it is more obvious).

I see, if I am not mistaken, the " element of the list with a linebreak preceding it" is a heuristic the jsonnet fmt uses ?

I can see the preceding linebreak's effect in this example, I think.

37     coll5: [{
38         name: "name",
39     }, someOther(), thirdObj(),
40     ],
41     coll6: [{
42                 name: "name",
43             }, someOther(),
44             thirdObj(),
45     ],

[sparkprime]

Related: #230

[sparkprime]

Yeah, it's implemented here https://github.com/google/jsonnet/blob/master/core/formatter.cpp#L1365

It may not be a good idea to have rules that complex, but it fixed some weirdnesses I was seeing in the cocktail examples.

[sparkprime]

[x, y, z] f(x, y, z) and (x + y + z) should all behave the same in this regard

[sparkprime]

So now I told you the heuristic, do you still thing it's surprising, and what behavior should it have?

(It's actually not really a heuristic since it's just one signal driving it)

[huggsboson]

Having thought about this for a few and watched a fair amount of jsonnet code evolve. I would probably say with at least the [ x, y, z, ] cases I'd love to see multi-line versions of them format as:

[
    {
        name: "value",
    }, 
    someOther(),
]

Which destroys the nice concise [{ }]. But formats really consistently as you add functions invocations in.

The one big counter examples are the env vars lists from kube:

[{
    key: "NAME",
    value: "value",
}, {
    key: "NAME2",
    value: "value2",
}]

But I think I'm fine with that being transformed to:

[
    {
        key: "NAME",
        value: "value",
    }, 
    {
        key: "NAME2",
        value: "value2",
    },
]

[sparkprime]

I'm going to try and generalize this argument. Where C[ ] is some "outer" construct which has sub-expressions that should be indented:

C[(
    ...
), (
    ...
)]

(extended to any number of sub-expressions)

Omitting the top-level indentation only works if every element is something that:

1. has no new line before some sort of parenthesis opening, and a new line after it
2. has a newline before the closing parenthesis, and no new line after it

Some obscure examples that would be OK:

[(
    ...
), [
    ...
], {
    ...
}, (
)]

((
    ...
) + [
    ...
] * func(
    ...
)]

func([
    ...
] + [
    ...
])

if (
    ...
) && (
    ...
) && (
    ...
) then [
    ...
] else [
    ...
]

[sparkprime]

A separate question is whether it should merely accept or actually dictate that form.

[sparkprime]

Oh additional rules:

a) the "outer" construct has to have no newline before its first sub-expression
b) the "outer" construct has to have no newline before its closing paren (if any).

[huggsboson]

yeah I was largely proposing not using the compact [{ and [( forms of things as they don't tend to format well if you have things like locals or functions instead of literals inline.

[sparkprime]

The original reported problem is fixed now. We didn't do everything we discussed here, but the discussion went much deeper than the original issue :)