Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format of multiple union sets #2958

Closed
egonbraun opened this issue Nov 26, 2020 · 5 comments
Closed

Format of multiple union sets #2958

egonbraun opened this issue Nov 26, 2020 · 5 comments

Comments

@egonbraun
Copy link

egonbraun commented Nov 26, 2020

Expected Behavior

I expect the format command to leave the below statement unchanged or a way to tell opa fmt to ignore it.

violations := aws_violations_for_environment_policy  |
              aws_violations_for_app_number_policy   |
              aws_violations_for_stack_name_policy   |
              aws_violations_for_stack_number_policy

Actual Behavior

The line gets changed to this, highly impacting the readability and maintainability of the code.

violations := ((aws_violations_for_environment_policy | aws_violations_for_app_number_policy) | aws_violations_for_stack_name_policy) | aws_violations_for_stack_number_policy

Steps to Reproduce the Problem

Create a file called policy.rego with the following content:

package example

aws_violations_for_environment_policy := {}

aws_violations_for_app_number_policy := {}

aws_violations_for_stack_name_policy := {}

aws_violations_for_stack_number_policy := {}

violations := aws_violations_for_environment_policy  |
              aws_violations_for_app_number_policy   |
              aws_violations_for_stack_name_policy   |
              aws_violations_for_stack_number_policy

Then you can run:

$ opa fmt policy.rego -w

OPA version

0.24.0

Additional Info

All the variables there are sets, their actual values are irrelevant to the issue being reported.

@anderseknert
Copy link
Member

Formatting is obviously subjective in nature, and how it should work with indentation multi-line assignment is an interesting question. One thing I very much agree on is that the formatter should be able to take line length into account. I've had a few cases where opa fmt resulted in several hundred characters being put on a single line, which I would consider bad form pretty much regardless of language used. But again, also that is subjective ;)

@egonbraun
Copy link
Author

egonbraun commented Nov 27, 2020

@anderseknert I agree with you, it's totally subjective. I am open to having a different style, mine was just a suggestion that works for me. Sorry for not making that clearer in my original message.

The main issue for me is that using multiple parentheses to separate the unions into "couples" and not taking into account the final line length makes the code quite hard to read and maintain.

Another suggestion, could be:

all_sets := (first_set | second_set) |
            (third_set | fourth_set) |
            (fifth_set | sixth_set) |
            (seventh_set | eigth_set) |
            (nineth_set | tenth_set)

Anyway, just some suggestions. :)

@stale
Copy link

stale bot commented Nov 22, 2021

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

@stale stale bot added the inactive label Nov 22, 2021
@stale stale bot removed the inactive label Dec 3, 2021
@stale
Copy link

stale bot commented Jan 2, 2022

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

@anderseknert
Copy link
Member

Closing in favor of #4508, where we'll try to compile all ideas for the next iteration of opa fmt (and where this idea is included).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants