fmt inconsistency formatting single attribute object with comment

[anderseknert]

setting := {
    # set to enable
    "enabled": true,
}

opa fmt ->

setting := {# set to enable
"enabled": true}

Even opa fmt seems to dislike its formatting in this case (as it should), because formatting once more changes the output to a third form:

opa fmt ->

setting := {"enabled": true} # set to enable

If there are many lines of comments in the object, the formatted result is even more weird:

setting := {
    # set to enable
    # or disable
    # who knows?
    "enabled": true,
}

opa fmt ->

setting := {# set to enable
# or disable
# who knows?
"enabled": true}

opa fmt ->

setting := {# or disable # set to enable
# who knows?
"enabled": true}

opa fmt ->

setting := {# who knows? # or disable # set to enable
"enabled": true}

opa fmt ->

setting := {"enabled": true} # who knows? # or disable # set to enable

[anderseknert]

Eventually consistent formatting :P

[tjons]

I'm painfully familiar with the formatter quirks from https://github.com/open-policy-agent/opa/pull/7169. The formatter works by extracting all comments from the AST and then walking the AST and reinserting them at what it thinks are block starts/ends. I think it's pretty clear that case #1 you mention should be left at the third form, right? Case #2 is less clear to me... I think I'd expect the formatter to either leave that comment alone or insert it all above the object without reordering the lines.

@anderseknert if you'll help me figure out intended behavior, I think I remember enough about the formatter to track this one down 😁

[anderseknert]

Thanks @tjons! Yes, having done some work on formatting myself, I know it's not as easy as it sometimes seems 😅

Needless to say, it's just my opinion, but I think both of these examples should be left as is by the formatter (assuming of course tabs are used for indentation, no trailing whitespace, and so on:

setting := {
    # set to enable
    "enabled": true,
}

setting := {
    # set to enable
    # or disable
    # who knows?
    "enabled": true,
}

If the formatter thinks the object without comments should be made more compact, i.e. this:

setting := {
    "enabled": true,
}

to turn into this:

setting := {"enabled": true}

Then that's fine with me. But if there's a comment (or more) inside, that should just get aligned with the attribute in the object. Moving it to the outside changes the "semantics" of the comment, and increases the risk of that exceeding reasonable line length limits.

And obviously more than anything, running one pass of opa fmt must always produce a final formatted state, and any subsequent invocations should be no-ops.

[johanfylling]

A related case where I think the formatter should avoid collapsing the object/set is when a comment is trailing a field/entry:

p := {
	"foo": "bar" # baz
}

q := {
	"foo" # bar
}

-x->

p := {"foo": "bar"} # baz

q := {"foo"} # bar

[stale[bot]]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.