-
Notifications
You must be signed in to change notification settings - Fork 201
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
chore: add scalafix #1700
chore: add scalafix #1700
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sensible to me, did you try running scalafix? I remember I had some issues with it, but I tried fixing some dependencies causing those issues, so might be ok now.
6676104
to
b442d1b
Compare
I temporarily removed |
846dd40
to
7136719
Compare
I think there's no point in adding scalafix to the CI for now - first we should handle initial sbt load overhead. |
We can just add it to a separate job, no? |
I'll do |
9f7f859
to
985a0b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for doing this!
We have to agree on scalafix settings. Currently, they are very similar to the ones that can be found in Metals.
What I've found is that
RemoveUnused
have strange behavior - sometimes it removes onlyval name =
and lefts right side of the assignment :/Moreover, we have to be very careful, for example here
bloop/frontend/src/main/scala/bloop/Cli.scala
Lines 508 to 518 in 1b6633a
there is a side effect (
runAsync
) that is assigned to the variable and we can't remove both sides. Probably the best way would be to iterate manually over those unused warnings.scalafix can be run on frontend by
frontend/scalafix
cc: @tgodzik