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

Add Scalafix rewrites for 1.0.0 #1793

Merged
merged 20 commits into from
Aug 9, 2017
Merged

Add Scalafix rewrites for 1.0.0 #1793

merged 20 commits into from
Aug 9, 2017

Conversation

gabro
Copy link
Contributor

@gabro gabro commented Aug 6, 2017

This is a WIP for #1791.

It's very preliminary, but feel free to share your thoughts, try this out and add more edge cases to the tests!

@gabro gabro changed the title Add RemoveUnapply and RemoveCartesianBuilder Scalafix rewrites Add Scalafix rewrites for 1.0.0 Aug 6, 2017
@codecov-io
Copy link

codecov-io commented Aug 7, 2017

Codecov Report

Merging #1793 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1793   +/-   ##
=======================================
  Coverage   94.88%   94.88%           
=======================================
  Files         241      241           
  Lines        4147     4147           
  Branches      107      107           
=======================================
  Hits         3935     3935           
  Misses        212      212

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96c1214...0311cc2. Read the comment docs.

@kailuowang
Copy link
Contributor

This is awesome. I suggest that we make it available to more user sooner.
If we can add a quick step by step instruction on how to use this, we can merge it now and link it from the migration guide. It doesn't have to include migrations for all the breaking changes. We can add those incrementally - that would also allow more people to contribute. We just need to communicate that this is an incomplete auto-migration.
What do you think?

@gabro
Copy link
Contributor Author

gabro commented Aug 7, 2017

I pushed another easy one (RenameFreeSuspend) and refactored some code a bit.

@kailuowang I agree that merging soon would be a good idea. About the strategy to make this available to the users I would like @olafurpg's opinion, since I think this is the first real-world scenario of making a rewrite available with a library and things are still a bit sketchy.

@olafurpg
Copy link
Contributor

olafurpg commented Aug 7, 2017

Great work @gabro!

The simplest way to share a rewrite at the moment is to put it in a single source file under scalafix/rewrites/src/main/scala/Cats_v1.scala and users can run it from the sbt shell with > scalafix github:typelevel/cats/v1 with the sbt-scalafix plugin. It's possible to download the file locally and run > scalafix file:path/to/Cats_v1.scala if people prefer that.

Alternatively, it's possible to publish the rewrites to Maven. As long as the artifacts are on the same classpath as scalafix then users can run the rewrites with > scalafix scala:fix.Cats_v1 (assuming rewrite fqn is fix.Cats_v1).

The sbt plugin is a thin shim over the scalafix cli so it should be possible to integrate with other build tools than sbt. Pants has a scalafix integration, but I'm not aware of plugins for gradle or maven.

I hope to release scalafix 0.5.0 later this week, I addressing a few issues in the sbt plugin first.

@kailuowang
Copy link
Contributor

thanks @gabro! I agree sbt-scalafix running using github src is the easiest.

@gabro
Copy link
Contributor Author

gabro commented Aug 8, 2017

I just had a chat with @olafurpg about the best strategy for this, and we came up with the following solution (using scalafix 0.5):

  • we'll keep all the rewrites separated, so that people can run them in isolation in case anything goes wrong
  • I'll generate a single file called Cats_v1_0_0.scala (or something like that) that's just the concatenation of all the single rewrites in a single file. This is currently the most convenient way of running all the rewrites with scalafix, although it involves duplicating the source (single rewrites + generated file)

This will allow users to run all the rewrites using the scalafix sbt plugin: sbt scalafix github:typelevel/cats/v1_0_0 🎉

We also drafted a solution for future versions of scalafix for making the process much simpler for both the rewrite authors and the users. @olafurpg will provide more details soon :)

@kailuowang as soon as the "generated file" approach is in place, I think we can merge this PR and keep on iterating in different PRs :)

@gabro
Copy link
Contributor Author

gabro commented Aug 8, 2017

My bad, I got some parts of that mixed up. There won't be any duplication, just a single Cats_v1_0_0.scala file containing all the rewrites. Coming soon :)

This allows using `sbt scalafix github:typelevel/cats/v1_0_0` for
invoking the rewrite.
@olafurpg
Copy link
Contributor

olafurpg commented Aug 8, 2017

Any recommendations for OSS repos where we can try out the migration?

@kailuowang kailuowang modified the milestone: 1.0.0-RC1 Aug 8, 2017
@kailuowang
Copy link
Contributor

https://github.com/stew/dogs might be a good candidate.

@gabro
Copy link
Contributor Author

gabro commented Aug 9, 2017

@kailuowang I've covered a few corner case of the available rewrites and added some instructions on how to run the fixes. Take a look ;)

@gabro
Copy link
Contributor Author

gabro commented Aug 9, 2017

Also, once this gets merged I would suggest to use the original issue as an aggregation point for bug reports and general feedback. I expect a lot of corner cases will be found.

@kailuowang
Copy link
Contributor

Looking great to me, thanks so much! 👍

@kailuowang
Copy link
Contributor

Would love to get this merge and start to benefit users and iterate.

@milessabin
Copy link
Member

LGTM. I'd like to see this merged ASAP :-)

@@ -0,0 +1,5 @@
addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.5.0-M1")
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is repeated :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, thank you!

@@ -0,0 +1,5 @@
addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.5.0-M1")
addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.5.0-M1")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once is good enough, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

most definitely! Good catch, thanks :)

@gabro
Copy link
Contributor Author

gabro commented Aug 9, 2017

Updated, I've removed the duplicated line in plugins and bumped scalafix to the latest 0.5.0-M2 version.

@kailuowang kailuowang merged commit b1da07e into typelevel:master Aug 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants