opa fmt 2.0

[anderseknert]

The opa fmt command is great! The current version however has not moved at the same pace as the rest of the project, and could benefit from a remake. Creating this ticket to collect ideas and feature requests for a possible next iteration of the opa fmt command.

Some initial ideas from myself and previous tickets on the topic:

• Opinionated formatting to follow best practices. Examples of this includes using assignment := and equality == operators rather than unification = where possible. Other possible ideas to explore could be reformatting old style iteration to use some and every, membership checks to use in, etc... i.e. define best practice for a domain and have the formatter apply those where possible.

Original ticket: #4076

• Allow formatting of snippets/statements, and not just entire modules—e.g.

  $ echo 'xs := {1,     3,2,3,4}' | opa fmt
  xs := {1, 2, 3, 4}

Original ticket: #4104

• Decide on formatting for multi-line expressions. This includes unions of sets, long chains of with statements in tests, etc. These currently don't look as good as when manually formatted, and could definitely be improved.

Original ticket: #2958

• Better consideration of how comments are preserved or moved around.

Original ticket: #1549

• Take line length into account. This is especially painful when dealing with large objects such as mocks in tests, where the formatter sometimes reformats a map to be compressed into a single long line, in extreme cases several hundred characters long.

• Allow a directive (like a comment) to have the formatter ignore a specific line or rule. This could be useful when manual formatting is preferred for e.g. a table of data where extra indents and whitespace helps with readability.

---

More suggestions are welcome!

[oren-zohar]

@anderseknert not sure if this one is included in the list, but what about controlling the indentation (spaces vs tabs)

[anderseknert]

Thanks @oren-zohar!

Yeah I think controlling (i.e. configuration of) anything at all (indentation, max line length, style preferences) is a topic that definitely should be up for discussion. go fmt famously does not allow that, and I think the opa fmt command is largely modeled after that, but there are many other formatting tools that do allow configuration, so.. I don't know 🤷😄

One obvious benefit of keeping a single coherent model for formatting is of course that any policy formatted with the tool will pass the formatter checks regardless of where it came from. And maintenance of the tool will likely be more cumbersome if configuration is added to the mix. Perhaps a middle way could be to make the code modular enough that it's easy for anyone to simply extract it and build their own customizations on top of it.

[anderseknert]

Sorting and grouping imports!

import data.aws.iam.user.excluded_principal_name
import data.assertions.assert_not_in
import data.test_helpers.create_with_properties
import future.keywords
import data.aws.iam.user.deny
import data.assertions.assert_in
import data.test_helpers.with_properties

=>

import future.keywords

import data.aws.iam.user.deny
import data.aws.iam.user.excluded_principal_name

import data.assertions.assert_in
import data.assertions.assert_not_in

import data.test_helpers.create_with_properties
import data.test_helpers.with_properties

[srenatus]

It would be nice to have both --fail and --diff at the same time. Right now, --fail trumps --diff.

[NitroCao]

Optimize long lines.

dropped_capabilities[affected] {
    ctrs := containers[_]
    affected = {"image": ctrs.image, "caps_dropped": {cap | cap := ctrs.securityContext.capabilities.drop[_]; count(cap) > 0}}
}

=>

dropped_capabilities[affected] {
    ctrs := containers[_]
    affected = {
        "image": ctrs.image,
        "caps_dropped": {cap |
            cap := ctrs.securityContext.capabilities.drop[_]
            count(cap) > 0
        },
    }
}

[oren-zohar]

It would be nice to have both --fail and --diff at the same time. Right now, --fail trumps --diff.

also --fail and --list please!

[anderseknert]

The formatting currently applied to else clauses, where opa fmt adds a body with an empty true in it, feels like a low-hanging fruit to improve. I.e:

number := 10 {
    input.foo
} else := 20

formats to ->

number := 10 {
    input.foo
} else := 20 {
    true
}

The body added to the else clause here is IMHO redundant, and does not help make the rule more readable.

EDIT: This was fixed in OPA v0.47.0 :)

[stale]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

[charlieegan3]

Related to StyraInc/rego-style-guide#19 merged today, might it make sense to automatically correct this? i.e.

 full_name := name {
     name := concat(", ", [input.first_name, input.last_name])
 }

 divide_by_ten(x) := y {
     y := x / 10
 }

Formats to:

full_name := concat(", ", [input.first_name, input.last_name])

divide_by_ten(x) := x / 10

[anderseknert]

@charlieegan3 it does make sense, although it's hard to say where the responsibilities of the formatter starts and ends. I could imagine this as a linter rule just as well. Well, if we had one, that is 😆

[stale]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

[stale]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

[shinebayar-g]

I'd like to add feature request for print width / line length feature. opa fmt is formatting multi line string to a really long line.

Before (please ignore + usage, that seems to be wrong)

After

[stale]

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.

[nikpivkin]

Would this apply to the formatting of objects that are passed as input to tests?

Before:

    inp := {
        "azure": {
            "database": {
                "mssqlservers": [{
                    "securityalertpolicies": [{
                        "disabledalerts": [{
                            "value": "Sql_Injection"
                        }]
                    }]
                }]
            }
        }
    }

After:

inp := {"azure": {"database": {"mssqlservers": [{"securityalertpolicies": [{"disabledalerts": [{"value": "Sql_Injection"}]}]}]}}}

[anderseknert]

Yes, it would for sure.

[stale]

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.