-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add RenameInjectProdAndCoproduct, RenameTupleApplySyntax and RemoveSplit Scalafix rewrites #1813
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1813 +/- ##
==========================================
+ Coverage 94.88% 94.88% +<.01%
==========================================
Files 241 241
Lines 4147 4148 +1
Branches 97 103 +6
==========================================
+ Hits 3935 3936 +1
Misses 212 212
Continue to review full report at Codecov.
|
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 👍
Just left one comment about the Split
syntax. Not sure if want to support that one as well?
|
||
import cats.implicits._ | ||
import cats.arrow.Split | ||
import cats.syntax.split._ |
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.
The split
syntax isn't actually used below, and if it was used it, this import would clash with cats.implicits._
.
If you want to test the split syntax you would have
import cats.syntax.split._ // afterwards => import cats.syntax.arrow._
import cats.instances.function._
val f = toLong split toDouble
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.
ah, good catch, thanks. That's a half-baked attempt that I accidentally left in there.
I just pushed a proper version of that fix.
By the way, it'd be nice if scalafix tests were run by the CI (so that these kind of mistake would be caught). Do you think it's possible to add them to the build?
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.
I think we should add them to the build. We can tackle it in another PR.
This PR adds these rewrites:
RenameInjectProdAndCoproduct
RenameTupleApplySyntax
RemoveSplit
Two of them are super easy thanks to a feature recently landed in Scalafix (
replaceSymbol
). For this reason I also upgraded Scalafix to the latest milestone release for 0.5.0.