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

feature: Allow to run all scalafix rules on a file #3996

Merged
merged 2 commits into from Jun 9, 2022

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Jun 5, 2022

Previously, it would not be possible to run sclafix inside metals aside from organize imports. Now, it's possible to run a list of predefined rules and ones added in settings.

Fixes #4005

Previously, it would not be possible to run sclafix inside metals aside from organize imports. Now, it's possible to run a list of predefined rules and ones added in settings.
@tgodzik
Copy link
Contributor Author

tgodzik commented Jun 7, 2022

I think the approach is as reasonable as it can get, though I still wonder if we should only run available rules instead of failing in that case 🤔 it might be actually more work for the user to add it a more complex setting. The best road would be to get the scalafix deps from the build tool, but we currently don't have that and we would need to change that everywhere that scalafix can be used (which might be a bunch of places)

I will try add more defaults based on the scalafix contributed rules pages.

@nolondil
Copy link

nolondil commented Jun 7, 2022

Hey, great to see that you're adding a way to run all scalafix rules!

With that, would it be easier for me to wire the diagnostics for scalameta/metals-feature-requests#230?

I was a bit stuck with wiring everything and downloading the dependency when I tried to tackle my feature request

@kubukoz
Copy link
Contributor

kubukoz commented Jun 7, 2022

Gonna get myself a snapshot with this and give it a go now :)

Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Very cool. This was just a cursorary glance from me. I want to play around with it a bit. I agree, I totally we could just pull these from the build tool since now if you are using a custom rule of sort you'll need to remember to not only include it in your build to use via sbt, but also in your Metals configuration.

only run available rules instead of failing

I think if this is possible, then I'd favor this. Because let's say you have 3 built-in rules and one custom one, it'd be nice to still be able to trigger and run those 3 built in ones and just leave out the custom once instead of failing the whole thing. Then we'd need to still figure out how to nicely show the user that info, something like "hey! We were able to run scalafix, but we couldn't find X rule. You'll need to add it to run it"

@kubukoz
Copy link
Contributor

kubukoz commented Jun 7, 2022

noob question, how can I trigger this command on a local server snapshot?

Copy link
Member

@dos65 dos65 left a comment

Choose a reason for hiding this comment

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

Great job!

I'm only thinking that it's not fully IDE integrated without highlighting.

I'm wondering if we can try to turn Patches not only into textEdits but also into code actions?
I mean, if you have an ExplicitResultType rule, you won't notice that you need to run scalafix.

@dos65
Copy link
Member

dos65 commented Jun 7, 2022

@kubukoz you also need to run extension from this branch

ckipp01 added a commit to ckipp01/nvim-metals that referenced this pull request Jun 9, 2022
Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Have been playing around with a bit, and didn't hit on anything funky yet! LGTM!

@tgodzik
Copy link
Contributor Author

tgodzik commented Jun 9, 2022

Hey, great to see that you're adding a way to run all scalafix rules!

With that, would it be easier for me to wire the diagnostics for scalameta/metals-feature-requests#230?

I was a bit stuck with wiring everything and downloading the dependency when I tried to tackle my feature request

You should be able to add a lintAll method that could get invoked after a compilation finished for a specific project, though ideally we would only run it for the files in the buffer 🤔 we wouldn't want to run it on everything int the workspace. Coming back to it I am thinking it would be best if the build tool provided the diagnostics, but that would most likely require scalafix to be a compiler plugin, which it is not currently. That's a tough topic 🤔

@tgodzik
Copy link
Contributor Author

tgodzik commented Jun 9, 2022

Very cool. This was just a cursorary glance from me. I want to play around with it a bit. I agree, I totally we could just pull these from the build tool since now if you are using a custom rule of sort you'll need to remember to not only include it in your build to use via sbt, but also in your Metals configuration.

The fastest way to support it would be making scalafix take the dependencies in the configuration file, which was already suggested by @olafurpg

This is done in order to simplify adding the dependencies to user settings in case multiple rules would be available in one dependency.
@tgodzik tgodzik merged commit 6f644ef into scalameta:main Jun 9, 2022
@tgodzik tgodzik deleted the run-all-rules branch June 9, 2022 16:36
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.

Run all/chosen Scalafix rules on source files.
6 participants