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

lsp/format,cmd/fix: Apply rego.v1 formatting by default #958

Merged
merged 4 commits into from
Aug 2, 2024

Conversation

charlieegan3
Copy link
Member

Summary of changes:

  • when running the fix cmd, the rego.v1 formatter will be run for files regardless of rego.v1 being imported.
  • when formatting on save in a language server client, the rego.v1 formatter will run, and all fixable violations will also be corrected at save time.

This is implemented by adding the concept of mandatory fixes, where these are run on all files even if there are no violations, before the linter based fixes are run.

LSP on save:

Untitled.mov

Fix cmd:
Screenshot 2024-08-01 at 18 04 54

This adds support to fixers to take fixes which will be run for all
files, regardless of linter violations.

This is intended to be useful for running using regal fix as an
alternative to opa fmt where files will be rego-v1'd regarless of them
containing import rego.v1.

Signed-off-by: Charlie Egan <charlie@styra.com>
This means that the default fixes will all be applied on save unless
disabled in config.

With the exception of rego-v1, where this is always run.

Signed-off-by: Charlie Egan <charlie@styra.com>
This means that all files will get the rego.v1 fix, regardless of the
violation being found.

Signed-off-by: Charlie Egan <charlie@styra.com>
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.

There are a few things to change here, like the mentioned possible nil dereference. And I I think we should ensure that fixes are either mandatory or not, but not both. But I can do that in a new PR. Thanks a lot for helping out here!

// return "null" as per the spec
return nil, nil
li := linter.NewLinter().
WithUserConfig(*l.loadedConfig).
Copy link
Member

Choose a reason for hiding this comment

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

This is a nil-dereference panic if no config is provided. I know because I did the same mistake on my branch 😆

@anderseknert anderseknert merged commit 27afe15 into StyraInc:main Aug 2, 2024
3 checks passed
@charlieegan3 charlieegan3 deleted the mandatory-fixes branch August 5, 2024 12:42
srenatus pushed a commit to srenatus/regal that referenced this pull request Oct 1, 2024
This adds support to fixers to take fixes which will be run for all
files, regardless of linter violations.

This is intended to be useful for running using regal fix as an
alternative to opa fmt where files will be rego-v1'd regarless of them
containing import rego.v1.

Signed-off-by: Charlie Egan <charlie@styra.com>
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.

2 participants