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

fixer: Regal fix command #653

Merged
merged 21 commits into from
Apr 18, 2024
Merged

fixer: Regal fix command #653

merged 21 commits into from
Apr 18, 2024

Conversation

charlieegan3
Copy link
Member

@charlieegan3 charlieegan3 commented Apr 15, 2024

At the moment it works like this:

regal $ cat test.rego
package wow

#comment

allow = 1 {
        input.foo == true
}
regal $ ./regal fix test.rego
3 fixes applied:
test.rego:
- no-whitespace-comment
- use-assignment-operator
- use-rego-v1
regal $ cat test.rego
package wow

import rego.v1

# comment

allow := 1 if {
        input.foo == true
}

Related to #92

cmd/lint.go Outdated Show resolved Hide resolved
@charlieegan3 charlieegan3 force-pushed the fix-flag branch 2 times, most recently from 62b6591 to 0460b60 Compare April 15, 2024 16:39
pkg/fixer/fixer.go Outdated Show resolved Hide resolved
pkg/fixer/fixer.go Outdated Show resolved Hide resolved
cmd/lint.go Outdated Show resolved Hide resolved
pkg/fixer/fixer.go Outdated Show resolved Hide resolved
pkg/fixer/fixer.go Outdated Show resolved Hide resolved
pkg/fixer/fixes/options.go Outdated Show resolved Hide resolved
pkg/fixer/fixes/fmt.go Outdated Show resolved Hide resolved
@charlieegan3
Copy link
Member Author

Also, interested to get thoughts on --fix vs regal fix. Having a dedicated command gives us more options for flags etc. but --fix is what might feel more familiar.

Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! I'm fine with the direction taken here 👍 Although I'm now leaning more and more towards providing a regal fix command for a cleaner separation of linting and transformations. I've left some comments here and there for further discussion.

cmd/lint.go Outdated Show resolved Hide resolved
pkg/fixer/fixer.go Outdated Show resolved Hide resolved
pkg/fixer/fixes/fmt.go Outdated Show resolved Hide resolved
pkg/fixer/fixes/options.go Outdated Show resolved Hide resolved
@charlieegan3 charlieegan3 force-pushed the fix-flag branch 3 times, most recently from ce211cb to 3e9faac Compare April 16, 2024 16:19
@charlieegan3 charlieegan3 marked this pull request as ready for review April 16, 2024 16:19
@charlieegan3
Copy link
Member Author

@anderseknert this is ready for review again. I am more happy with the implementation here now. If you agree on the direction & interfaces etc, then I think we can get this in.

We might want to add code actions for the whitespace comment and assignment operator too, but these could happen as a follow on PR since this is already kinda large.

@charlieegan3 charlieegan3 changed the title fixer: First draft implementation of --fix fixer: First draft implementation of regal fix command Apr 16, 2024
@charlieegan3 charlieegan3 changed the title fixer: First draft implementation of regal fix command fixer: Regal fix command Apr 16, 2024
Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work on this! The code is really clean, and I had no problems whatsoever following along exactly in what it does. I did have a few thoughts and questions, but this is definitely getting there 😍 Next release is gonna be 💥

cmd/fix.go Outdated Show resolved Hide resolved
cmd/fix.go Outdated Show resolved Hide resolved
cmd/fix.go Outdated Show resolved Hide resolved
pkg/fixer/fixer.go Outdated Show resolved Hide resolved
pkg/fixer/fixer.go Outdated Show resolved Hide resolved
pkg/fixer/fixes/nowhitespacecomment.go Outdated Show resolved Hide resolved
pkg/fixer/fixes/nowhitespacecomment_test.go Outdated Show resolved Hide resolved
pkg/fixer/fixes/useassignmentoperator.go Outdated Show resolved Hide resolved
pkg/fixer/reporter.go Outdated Show resolved Hide resolved
@charlieegan3 charlieegan3 force-pushed the fix-flag branch 2 times, most recently from c527195 to 68e3cbe Compare April 17, 2024 15:41
@charlieegan3
Copy link
Member Author

This is ready for another review. I think we are getting there. Main changes:

  • The fixer will now run repeatedly until there are no fixes applied fixer: Regal fix command  #653 (comment)
  • Interfaces for fixing are now more generic and should better support other fix operations such as renaming.

Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terrific! Only some nits, but none of them blocking, so go on and merge when you feel done! 👏

This will be an awesome addition to Regal, and will save a lot of time both for us and others.

pkg/fixer/fixer.go Show resolved Hide resolved
pkg/fixer/fixes/fixes.go Show resolved Hide resolved
pkg/fixer/fixes/fmt.go Outdated Show resolved Hide resolved
@anderseknert anderseknert linked an issue Apr 17, 2024 that may be closed by this pull request
WIP while I work on some other fixes to prove out the interfaces.

Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
This makes it possible to set the filename and contents in the same
place.

Next is to standardize the result of a fix too.

Signed-off-by: Charlie Egan <[email protected]>
This should allow us to support other type of fix in future

Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
Fixes are run repeatedly now and this is not used.

Signed-off-by: Charlie Egan <[email protected]>
This is to make it more consistent with rule names

Signed-off-by: Charlie Egan <[email protected]>
@charlieegan3 charlieegan3 merged commit dfd9ee2 into main Apr 18, 2024
@charlieegan3 charlieegan3 deleted the fix-flag branch April 18, 2024 08:28
charlieegan3 added a commit that referenced this pull request Apr 18, 2024
#653 added fixes for
no-whitespace-comment and use-assignment-operator. This PR adds code
actions for these in the regal lsp.

Signed-off-by: Charlie Egan <[email protected]>
charlieegan3 added a commit that referenced this pull request Apr 18, 2024
#653 added fixes for
no-whitespace-comment and use-assignment-operator. This PR adds code
actions for these in the regal lsp.

Signed-off-by: Charlie Egan <[email protected]>
charlieegan3 added a commit that referenced this pull request Apr 18, 2024
#653 added fixes for
no-whitespace-comment and use-assignment-operator. This PR adds code
actions for these in the regal lsp.

Signed-off-by: Charlie Egan <[email protected]>
charlieegan3 added a commit that referenced this pull request Apr 18, 2024
* lsp: Implement code actions for new fixes

#653 added fixes for
no-whitespace-comment and use-assignment-operator. This PR adds code
actions for these in the regal lsp.

Signed-off-by: Charlie Egan <[email protected]>

* Move commands under a fix namespace

Signed-off-by: Charlie Egan <[email protected]>

---------

Signed-off-by: Charlie Egan <[email protected]>
srenatus pushed a commit to srenatus/regal that referenced this pull request Oct 1, 2024
`regal fix` is a new command that is capable of fixing some linter errors:

- opa-fmt
- use-rego-v1
- no-whitespace-comment
- use-assignment-operator

Fixes are applied when the input files fail with linter errors. User config is supported.

The same fix logic is also used from the language server for code actions.
srenatus pushed a commit to srenatus/regal that referenced this pull request Oct 1, 2024
* lsp: Implement code actions for new fixes

StyraInc#653 added fixes for
no-whitespace-comment and use-assignment-operator. This PR adds code
actions for these in the regal lsp.

Signed-off-by: Charlie Egan <[email protected]>

* Move commands under a fix namespace

Signed-off-by: Charlie Egan <[email protected]>

---------

Signed-off-by: Charlie Egan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remediation
2 participants