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

The compact output format should include a summary #1364

Open
anderseknert opened this issue Jan 23, 2025 · 17 comments
Open

The compact output format should include a summary #1364

anderseknert opened this issue Jan 23, 2025 · 17 comments
Labels
good first issue Good for newcomers tooling

Comments

@anderseknert
Copy link
Member

> regal lint --format compact PowerShell/ScubaGear/Rego/TeamsConfig.rego        
+--------------------------------------------------+-------------------------------------------+
|                     Location                     |                Description                |
+--------------------------------------------------+-------------------------------------------+
| PowerShell/ScubaGear/Rego/TeamsConfig.rego:1:9   | Directory structure should mirror package |
| PowerShell/ScubaGear/Rego/TeamsConfig.rego:397:5 | Pointless reassignment of variable        |
| PowerShell/ScubaGear/Rego/TeamsConfig.rego:458:5 | Pointless reassignment of variable        |
| PowerShell/ScubaGear/Rego/TeamsConfig.rego:297:5 | Pointless reassignment of variable        |
| PowerShell/ScubaGear/Rego/TeamsConfig.rego:366:5 | Pointless reassignment of variable        |
| PowerShell/ScubaGear/Rego/TeamsConfig.rego:340:5 | Pointless reassignment of variable        |
| PowerShell/ScubaGear/Rego/TeamsConfig.rego:61:5  | Pointless reassignment of variable        |
| PowerShell/ScubaGear/Rego/TeamsConfig.rego:185:5 | Pointless reassignment of variable        |
| PowerShell/ScubaGear/Rego/TeamsConfig.rego:530:5 | Pointless reassignment of variable        |
| PowerShell/ScubaGear/Rego/TeamsConfig.rego:557:5 | Pointless reassignment of variable        |
| PowerShell/ScubaGear/Rego/TeamsConfig.rego:504:5 | Pointless reassignment of variable        |
| PowerShell/ScubaGear/Rego/TeamsConfig.rego:35:5  | Pointless reassignment of variable        |
+--------------------------------------------------+-------------------------------------------+

While the compact format should be compact, we can afford to include a one-liner summary similar to what the pretty formatter outputs.

@anaypurohit0907
Copy link

hi i would like to work on this issue, please guide me.

@anderseknert
Copy link
Member Author

Hi there! I’d be happy to. Some general advice for contributing can be found here.

The code for the reporters is found here. Let me know if you have any specific questions you need answered! Also feel free to join the Styra slack and the Regal channel if you want to discuss this there.

@anderseknert anderseknert changed the title The compact output format should incude a summary The compact output format should include a summary Jan 25, 2025
@anaypurohit0907
Copy link

thanks a lot for the code, ill go through it and suggest my approach to doing this.

@anderseknert
Copy link
Member Author

Thanks! And don't worry if you get stuck somewhere – let us know!

As for this particular issue, I'd probably create a small example policy with a few violations in it to lint, like:

p.rego

package p

camelCase := "hello"

foo := true

bar := foo

Then you can run your checked out Regal-code to lint it with the compact formatter:

> go run main.go lint -f compact p.rego
+------------+-------------------------------------------+
|  Location  |                Description                |
+------------+-------------------------------------------+
| p.rego:1:9 | Directory structure should mirror package |
| p.rego:7:1 | Pointless reassignment of variable        |
| p.rego:3:1 | Prefer snake_case for names               |
| p.rego:5:1 | Metasyntactic variable name               |
| p.rego:7:1 | Metasyntactic variable name               |
+------------+-------------------------------------------+

If you change the format to pretty you'll see the last line saying:

1 file linted. 5 violations found.

That's pretty much what we should have here too, and I'm sure you can borrow some code from the pretty formatter to accomplish that. The most difficult part is probably to make it look native to the compact format. Ideally it'd look something like this:

+------------+-------------------------------------------+
|  Location  |                Description                |
+------------+-------------------------------------------+
| p.rego:1:9 | Directory structure should mirror package |
| p.rego:7:1 | Pointless reassignment of variable        |
| p.rego:3:1 | Prefer snake_case for names               |
| p.rego:5:1 | Metasyntactic variable name               |
| p.rego:7:1 | Metasyntactic variable name               |
+------------+-------------------------------------------+
| 1 file linted |  5 violations found. | 
+------------+-------------------------------------------+

But obviously, with all the columns aligned, lol

@anaypurohit0907
Copy link

anaypurohit0907 commented Jan 25, 2025

(lol we both commented at the same time, i read your comment, and yes i do need to formatt the print statement to align the table)

I did some reading and heres what i think :

  1. we can get the number of files scanned and number of violations from the r.Summarry object.
  2. format that data and print it after the table.render() and print it.

so it would look something like this:

  `  //inside CompactReporter {
    .
    .
    .

       table.Render()

//printing the summary

summary := fmt.Sprintf("%d files Linted, %d violations found.",
	r.Summary.FilesScanned, 
	r.Summary.NumViolations)

_, err := fmt.Fprintln(tr.out, strings.TrimSuffix(sb.String(), " "), summary)

return err`

What do you think?

@anaypurohit0907
Copy link

anaypurohit0907 commented Jan 25, 2025

@anderseknert i did some testing with different ways we can print , but all felt non-native, even the example you have given feels like that,

if it were up to me i would prefer the summary to be seperate from the table like so:

+------------+-------------------------------------------+
|  Location  |                Description                |
+------------+-------------------------------------------+
| p.rego:1:9 | Directory structure should mirror package |
| p.rego:7:1 | Pointless reassignment of variable        |
| p.rego:3:1 | Prefer snake_case for names               |
| p.rego:5:1 | Metasyntactic variable name               |
| p.rego:7:1 | Metasyntactic variable name               |
+------------+-------------------------------------------+
 1 file linted ,  5 violations found. 

what do you think?

here are some of the formats i tried :

Image

Image

this ones super weird ik :

Image

this is the one i liked best :

Image

heres another images of the same with different file :

Image

@anderseknert
Copy link
Member Author

I liked the example I gave (minus of course that it was written on my phone and looked like crap) mostly for that it would have a footer line similar to how the table has a header line. And it would make sense for the number of files linted to be reported below the name of the files. But I’m not sure the library allows a “footer” section like how it allows a header. Markdown certainly doesn’t.

So yes, I agree. Of the alternatives you tried (and thanks for presenting them so clearly!) I too prefer the one you suggested. So if the footer option isn’t feasible, let’s go with that 👍

@anaypurohit0907
Copy link

cool!, Let me try my best to implement the footer first, if it doesnt work out ill go with the option i presented.

thanks a lot for guiding me .

@anderseknert
Copy link
Member Author

anderseknert commented Jan 25, 2025

Thanks! And I appreciate it. Don’t spend too much time on it though, as what you have is infinitely better than what we currently do :) Looking forward to review your PR!

@anaypurohit0907
Copy link

@anderseknert i think we can append to the table itself!!

heres 2 more options:

Image

or this? :

Image

@anderseknert
Copy link
Member Author

Neat!

And it’s almost there. Just slightly off with no “+” where the lines intersect in the first example, or the line looking different in the summary for the other example.

But I was kinda expecting that making it look native would involve all sorts of hacks. If you want to spend your weekend getting that to look perfect, I’m not gonna stop you, lol. But really, the alternative you suggested is 100% good enough.

@anaypurohit0907
Copy link

so i looked at the setfooter definition and we can set it, it looks something like this:

Image

the only thing bugging me it the violations being center aligned, and i dont think we can change that.

@anderseknert
Copy link
Member Author

Wow, you’re fast! The centered alignment is curious, but doesn’t bother me. Which one do you like the best yourself? Whichever you pick, we’ll go with it.

@anaypurohit0907
Copy link

letting me choose? og boy, this is the hardest decision in my life.

after consulting my roomates , they said this one looked better :

Image

@anderseknert
Copy link
Member Author

All settled then! If your room mates have GitHub handles, mention them in the PR description and I’ll credit them in the next release changelog :)

Thanks a lot for contributing to Regal! And welcome back anytime. Would love to work with you again.

@anaypurohit0907
Copy link

My roomates arent really cs grads, ones in mechatronics.

thanks a lot for letting me work on this and guiding me through this, ill make a pr.

(ps. this is my first pr to a big code repo im so happy)

@anderseknert
Copy link
Member Author

I have no formal CS education either, and I used to sell hi-fi equipment before I got my first developer job. I don’t judge :)

Knowing I helped you to your first open source contribution is 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers tooling
Projects
None yet
Development

No branches or pull requests

2 participants