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

Scalafix 0.18 to 0.20 #2214

Merged
merged 45 commits into from Nov 5, 2018

Conversation

Projects
None yet
5 participants
@amarrella
Contributor

amarrella commented Oct 29, 2018

Attempts a first stab at #2045

It's not much yet but feel free to give feedback / suggestions.
It's my first time ever writing scalafix rules.

Also, feel free to discard it completely if you have something better already

amarrella added some commits Oct 29, 2018

@rossabaker

Wow, this is great to see. No, we don't have anything like it, and I'm so glad it's coming to fruition.

Another one that has tripped several people up is the name change in MiMe types since we started using the MimeDb class: application/html turns into application.html, for example.

I never had any large 0.18 apps, so this is something the community can say more about than me. But I'm anxious to just have this infrastructure in place so people can enhance it with more rules.

amarrella added some commits Oct 29, 2018

@SystemFw

This comment has been minimized.

Contributor

SystemFw commented Oct 30, 2018

This is great!

@amarrella

This comment has been minimized.

Contributor

amarrella commented Oct 30, 2018

Thanks for the nice words :) I have a few questions:

  • I think we should have only 1 rule to fix from 0.18 to 0.20 since it will be a one-off job. What do people think? We can still keep the code clean by refactoring but I think running a single command would be a nicer experience than running them in a sequence.
  • For the withBody -> withEntity fix do we want to change the type or should we wrap the foo.withEntity in some kind of F.pure?
  • For the client i'm working my way to it here amarrella#1 There is a bit of complexity caused by the fact that .apply disappears in favor of resource. I think it will be really hard to automatically refactor the user's code to use resource. Are we fine in transforming it into a .resource and let the user follow the compiler errors to refactor?
  • For the resource/stream now we need at least a ContextShift available (if not a ConcurrentEffect, if we are not using IO). For now I think i'll just make the user do that change as well following the compile errors. We can improve later i think, at least for people that use IO. Generic code becomes a little trickier.

I know that the code is a bit messy, but bear with me, I promise it will be much cleaner soon :)

amarrella added some commits Oct 30, 2018

homepage := Some(url("https://github.com/http4s/http4s")),
licenses := List("Apache-2.0" -> url("http://www.apache.org/licenses/LICENSE-2.0")),
developers := List(
Developer(

This comment has been minimized.

@amarrella

amarrella Oct 30, 2018

Contributor

About this: I actually don't care about being marked as the developer or anything. So please change it to work with your release process :)

This comment has been minimized.

@rossabaker

rossabaker Nov 1, 2018

Member

This doesn't touch the main build.sbt at all. Would we deploy this as a submodule, or is it just a project-in-a-project?

This comment has been minimized.

@amarrella

amarrella Nov 1, 2018

Contributor

What you prefer, I didn't think about deployment at all and I don't know if it's best as a submodule or a project in a project. Your call :)

@amarrella

This comment has been minimized.

Contributor

amarrella commented Oct 30, 2018

I published a snapshot (under my name for now) and added a readme, try it out :)
186d6cd

@rossabaker

This comment has been minimized.

Member

rossabaker commented Nov 1, 2018

We're thinking about another milestone tomorrow. It would be neat to get this out. I will give it a more thorough review when the workday is done.

I am ignorant on scalafix: if we try to implement rules for an 0.18.20 -> 0.20.0-M2 project, can it be run to go from 0.20.0-M1 to 0.20.0-M2? Or does it only work between known endpoints?

@amarrella

This comment has been minimized.

Contributor

amarrella commented Nov 1, 2018

It's totally independent from the version. scalafix just looks at your code, sees what's in there and changes it based on the rule. I called the rule Http4s018To020 because it contains the changes to go from 0.18 to 0.20, but you can run it on 0.17 or on akka-http code if you want :D

Obviously if the api changes between 0.20-M1 and 0.20-M2 we will need to touch the rules to reflect the changes, but that's about it

At the moment the rules are a bit raw and can definitely be improved, but I think a few decisions on the points above can be made and can be handled by a separate PR :)

@rossabaker

This comment has been minimized.

Member

rossabaker commented Nov 1, 2018

I think we should have only 1 rule to fix from 0.18 to 0.20 since it will be a one-off job. What do people think? We can still keep the code clean by refactoring but I think running a single command would be a nicer experience than running them in a sequence.

I agree with this, especially if it's still helpful for people migrating from milestones in between. If not, well, we can readjust our strategy.

For the withBody -> withEntity fix do we want to change the type or should we wrap the foo.withEntity in some kind of F.pure?

I bet F.pure is easier for a migration that still compiles, but it no longer needs to be in F. I'd like to encourage people to not keep that in F, but I'd also like to make it easy for people to migrate. I'll delegate to you on which is smarter here, but that's the tradeoff.

For the client i'm working my way to it here amarrella#1 There is a bit of complexity caused by the fact that .apply disappears in favor of resource. I think it will be really hard to automatically refactor the user's code to use resource. Are we fine in transforming it into a .resource and let the user follow the compiler errors to refactor?

This is similar to the previous question in terms of tradeoffs. I would like to encourage .resource, but .allocate would give something that compiles. (Replacing shutdown is trickier, though.)

For the resource/stream now we need at least a ContextShift available (if not a ConcurrentEffect, if we are not using IO). For now I think i'll just make the user do that change as well following the compile errors. We can improve later i think, at least for people that use IO. Generic code becomes a little trickier.

Migrating StreamApp to IOApp is out of scope since those aren't our types, right? Because if we open that door, then we're talking about cats-effect and fs2 migrations.

Sorry if these are redundant with the code. Just trying to provide some overdue answers until I have time to look deeper.

@rossabaker

This comment has been minimized.

Member

rossabaker commented Nov 1, 2018

Obviously if the api changes between 0.20-M1 and 0.20-M2 we will need to touch the rules to reflect the changes, but that's about it

There will be some modest changes between 0.20.0-M1 and 0.20.0-M2, and probably again before 0.20.0. But I expect those to be much smaller than what's already covered.

@amarrella

This comment has been minimized.

Contributor

amarrella commented Nov 1, 2018

I bet F.pure is easier for a migration that still compiles, but it no longer needs to be in F. I'd like to encourage people to not keep that in F, but I'd also like to make it easy for people to migrate. I'll delegate to you on which is smarter here, but that's the tradeoff.

I'm also for encouraging people. Following the compile errors should be enough. Worst case they can add the F.pure themselves

This is similar to the previous question in terms of tradeoffs. I would like to encourage .resource, but .allocate would give something that compiles. (Replacing shutdown is trickier, though.)

I didn't even know about .allocate! Again though, I'm + for encouraging people to do the right thing, though i expect it to be one of the most painful things to change in a codebase on migration (if they weren't using .stream. I think if people already used .stream they will be fine either way. I don't know the percentages though. Your call

Migrating StreamApp to IOApp is out of scope since those aren't our types, right? Because if we open that door, then we're talking about cats-effect and fs2 migrations.

Yes you are right (I might actually create a separate scalafix for fs2 StreamApp to IOApp :) )

There will be some modest changes between 0.20.0-M1 and 0.20.0-M2, and probably again before 0.20.0. But I expect those to be much smaller than what's already covered.

That's fine, we can just update the rule while we do it. This opens a question wrt versioning as well. Since the http4s versions (major & minor) themselves are in the rule name, we could just have an independent semver for the rule (since rules will evolve at a different pace than the rest of the project, i assume )

amarrella added some commits Nov 2, 2018

amarrella added some commits Nov 2, 2018

@amarrella

This comment has been minimized.

Contributor

amarrella commented Nov 3, 2018

For now the instructions to run this are:

  1. Make sure your scala version is >= 2.11.12 or 2.12.7 (no 2.13 yet)
  2. Add the scalafix plugin to your project/plugins.sbt
    addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.9.0")
  3. Add this to your build.sbt
scalafixDependencies in ThisBuild += "org.http4s" %% "http4s-scalafix" % http4s020Version
  1. Run
    sbt ";scalafixEnable; scalafix Http4s018To020"

I was hoping to find a simpler (one command to run) way that didn't make users change their build and plugins but for now that's the easiest i found.

Do you want to add this somewhere in the docs? Perhaps in the first page https://http4s.org/v0.20/ under a Migration from http4s 0.18 title so that users don't have to look for it?

I'm sorry for the many questions! Anyways besides that, I think the pr is ready now if you want to experimentally add it to M2 :)

EDIT: simplified command (thanks @olafurpg)

@rossabaker

This comment has been minimized.

Member

rossabaker commented Nov 3, 2018

Yes, I think a page on the docs for this would be great. Just an md in docs/src/main/tut -- even if there isn't any code to compile, that's the right way to add a new page. Would you want to give that a try or should we leave it for a subsequent PR?

I still need to go through the code itself, but this is work is very much needed and appreciated. Thank you for a great PR.

@rossabaker

I'm very excited about this one. A couple outstanding questions, but nothing I'd block a release on. I think this is going to help a lot of people.

Show resolved Hide resolved build.sbt
Show resolved Hide resolved project/Http4sPlugin.scala Outdated
Show resolved Hide resolved project/plugins.sbt Outdated
Show resolved Hide resolved project/plugins.sbt
Show resolved Hide resolved scalafix-tests/src/test/scala/fix/RuleSuite.scala
Show resolved Hide resolved scalafix/src/main/scala/fix/MimeRules.scala
@amarrella

This comment has been minimized.

Contributor

amarrella commented Nov 3, 2018

Thanks :) I really that it will help people in the migration! This library is fantastic and honestly can't wait enough for 0.20 :)

I'll address a few of your comments now and I'll push a small doc as well. That can be improved in a separate PR but I would love for people to use it in M2 so I can see what can be improved (maybe with the help of other contributors as well) before the final release, so I feel it's important to provide some docs at least.

Just fyi, I opened a fs2 pr (just for StreamApp for now) which should help in the migration as well. If this doesn't get merged to fs2, i'll publish it separately but should be available soon as well.

amarrella added some commits Nov 3, 2018

@amarrella

This comment has been minimized.

Contributor

amarrella commented Nov 3, 2018

I added the doc, though I was unable to preview it. I set weight 2 so it should be just after quickstart. If that's not ok takes a second to change :)

I did sbt "; project docs; tut; makeSite; previewSite" but didn't work (even though the docs compiled fine - I think I have the exact same issue as #1718 ).

amarrella added some commits Nov 3, 2018

@olafurpg

Great job @amarrella! Thank you for working on this, looks good overall 👍 Let me know if you have any questions.

For the record, the scalafix rules don't have to live in the same sbt build as the main project. The rules can live in an isolated build under scalafix/build.sbt if you prefer to keep things separated.

There can also be multiple input/output projects, which might come in handy once you implement migration rules for several http4s versions.

Show resolved Hide resolved scalafix-tests/src/test/scala/fix/RuleSuite.scala
```
3. Add this line to your `build.sbt` and upgrade the http4s version to the latest 0.20 release.
```scala
scalafixDependencies in ThisBuild += "org.http4s" %% "http4s-scalafix" % http4s020Version

This comment has been minimized.

@olafurpg

olafurpg Nov 4, 2018

I agree it would be nice to have a one-liner syntax from the shell, I opened scalacenter/scalafix#905 to discuss adding this functionality.

object HttpServiceRules {
def unapply(t: Tree): Option[Patch] = t match {
case t @ Type.Name("HttpService") => Some(Patch.replaceTree(t, "HttpRoutes"))

This comment has been minimized.

@olafurpg

olafurpg Nov 4, 2018

It would be good to use a symbol matcher here

val httpServer = SymbolMatcher....
// ...
case httpServer(t @ Type.Name(_)) =>

so that you know this resolves to the http4s symbol (even if it's renamed) and not HttpService from some other library

This comment has been minimized.

@amarrella

amarrella Nov 4, 2018

Contributor

Right, awesome! I'll commit those changes now

@amarrella

This comment has been minimized.

Contributor

amarrella commented Nov 4, 2018

Great job @amarrella! Thank you for working on this, looks good overall 👍 Let me know if you have any questions.

Thanks, and thank you for reviewing it :) Scalafix is an amazing tool, the more I use it the more I like it :D

For the record, the scalafix rules don't have to live in the same sbt build as the main project. The rules can live in an isolated build under scalafix/build.sbt if you prefer to keep things separated.

Yeah I started this PR with giter8 template and it lived in a separate folder, though having it in the same build will probably make publishing easier. I see though that cats went towards the independent directory route. I'm fine with either, whatever is the easiest.

There can also be multiple input/output projects, which might come in handy once you implement migration rules for several http4s versions.
👍

@rossabaker

This comment has been minimized.

Member

rossabaker commented Nov 5, 2018

I still kind of like the single build, so we can publish these using the machinery that we already have in place. But as noted earlier, tweaking the scalafix rules will generally require a patch release of the main project. It looks like the cats one is run through publishLocal, so maybe that could work for people between releases?

@aeons

This comment has been minimized.

Member

aeons commented Nov 5, 2018

@amarrella This is awesome, thanks a lot!

@rossabaker

This comment has been minimized.

Member

rossabaker commented Nov 5, 2018

Any reason we shouldn't merge this now?

@amarrella

This comment has been minimized.

Contributor

amarrella commented Nov 5, 2018

Any reason we shouldn't merge this now?

Not from me :)

@rossabaker rossabaker merged commit ca96d51 into http4s:master Nov 5, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment