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

Deprecates Client#fetch #3372

Closed
wants to merge 7 commits into from
Closed

Conversation

satorg
Copy link
Contributor

@satorg satorg commented Apr 30, 2020

Fixes #3345

Tasks to complete:

  • Add @deprecate annotation to these Client methods:
    def fetch[A](req: Request[F])(f: Response[F] => F[A]): F[A]
    def fetch[A](req: F[Request[F]])(f: Response[F] => F[A]): F[A]
  • Fix all entrances of fetch in the project code (including tests) by replacing it with run followed by use1.
  • Add scalafix migration rules2,3.

1: There were some test cases which lost their sense after all the replacements because they turned into identical cases for the run method. Such cases were deleted.

2: (UPD: RESOLVED) Initially the scalafix subproject had the following dependency versions on http4s:

  • input: "0.18.21"
  • output: "0.20.0-M5"

To elaborate scalafix rules for this PR, the versions above were updated to "0.21.4" for both input and output. The old rules were removed.

If we wish to preserve the old rules, I see two ways to achieve it:

  1. Create multiple pairs of input and output modules for every http4s version we want to handle.
  2. Keep a single pair of input and output, but add versioning to rules which would allow to publish and then reference it by its desired version.

I'd assume that p.2 is more preferable and easy to manage, but I'm open to discussion.

3: Not all possible cases are handled in the rules mostly due to scalafix limitations. It includes cases where typeclasses like Bracket or FlatMap are not available for F[_] in a call site of fetch, but they're necessary for a new statement which replaces the old one. Unfortunately, scalafix doesn't allow to check whether some typeclass available for a particular type in a document tree or not. See this comment for an example.

@satorg
Copy link
Contributor Author

satorg commented Apr 30, 2020

I'd like to get confirmations on 1 and 2 if I can do it:

  1. Remove the TODO-ed test cases.
    UPD.: I decided to voluntarily remove these test cases which I believe have no sense anymore.
  2. Remove obsolete migration rules and update http4s versions in scalafix to more relevant values.
    UPD.: Done, although still disputable.

@rossabaker, WDYT?

@satorg satorg marked this pull request as draft Apr 30, 2020
@satorg satorg force-pushed the deprecate-client-fetch branch 2 times, most recently from 4e3a158 to 9940a6a Compare May 4, 2020
Copy link
Member

@rossabaker rossabaker left a comment

Exciting work! A couple ignorant comments below.

@@ -24,6 +24,7 @@ trait Client[F[_]] {
* response body afterward will result in an error.
* @return The result of applying f to the response to req
*/
@deprecated("Use run(req).use(f)", "0.21")
Copy link
Member

@rossabaker rossabaker May 8, 2020

Choose a reason for hiding this comment

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

We usually put patch versions in these, so 0.21.5.

@@ -27,29 +27,32 @@ lazy val rules = project.settings(
libraryDependencies += "ch.epfl.scala" %% "scalafix-core" % V.scalafixVersion
)

val http4sInputVersion = "0.21.4"
val http4sOutputVersion = "0.21.4"
Copy link
Member

@rossabaker rossabaker May 8, 2020

Choose a reason for hiding this comment

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

0.18 is ancient, but it's also a shame to lose the old rules. Is there a way to version these, so people might run multiple iterations if catching up across multiple versions? Probably related to these "semantic rules," but I would need to refresh my memory how these work.

Copy link
Contributor Author

@satorg satorg May 8, 2020

Choose a reason for hiding this comment

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

The problem is that the old rules don't even compile with the newer http4s version. I'd also prefer to keep the old rules but have no idea how to achieve it easily. But I'm open to any suggestions:)

Copy link
Member

@rossabaker rossabaker May 8, 2020

Choose a reason for hiding this comment

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

Perhaps @amarrella remembers how this works, as the original contributor. If not, we can learn together. 😄

Copy link
Contributor

@amarrella amarrella May 8, 2020

Choose a reason for hiding this comment

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

It's a long time since I've looked at these but can we perhaps set the old version as a dependency instead of depending on the project?

Copy link
Contributor Author

@satorg satorg May 9, 2020

Choose a reason for hiding this comment

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

@amarrella sorry, not sure if I caught it. Could you explain your idea a bit more please?

Copy link
Contributor

@amarrella amarrella May 11, 2020

Choose a reason for hiding this comment

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

@satorg apologies, I misunderstood the problem. I see the issue now with keeping http4sInputVersion = 0.18.x :(
Perhaps the cleanest version is getting rid of the rules like you did, and point users to use a older version of the jar if they need the older rule and then updating to the new one. I know it's way less ergonomic though :/

Copy link
Contributor Author

@satorg satorg May 11, 2020

Choose a reason for hiding this comment

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

Well, I think we also could convert the scalafix directory into a multi-module project where each module has its own set of dependencies. But I'm not sure if it's a really good way to go.

@satorg satorg force-pushed the deprecate-client-fetch branch 2 times, most recently from 2b63235 to 6cb704f Compare May 11, 2020
@satorg
Copy link
Contributor Author

satorg commented May 11, 2020

I think I almost done with scalafix rules. But there's still a question about what should we do with the old rules.

BTW, once scalafix rules are implemented how can I apply them to the base http4s project's sources? cc @rossabaker, @amarrella

@satorg satorg force-pushed the deprecate-client-fetch branch 6 times, most recently from 212ee99 to 5d9de5c Compare May 14, 2020
@rossabaker
Copy link
Member

rossabaker commented May 14, 2020

That's a great question on how to run them on our own sources. Can you use --tool-classpath to point to your local build? I confess it's been a while since I've run them, and I'm behind on the state of the art.

@satorg satorg force-pushed the deprecate-client-fetch branch 2 times, most recently from 6f8058e to f2a25a1 Compare May 18, 2020
@satorg
Copy link
Contributor Author

satorg commented May 18, 2020

@rossabaker I think I finished the scalafix rules for this task.
I wrote a bunch of rules for different cases – could you check it out please?
Let me know if you come up with some uncovered cases please.

@satorg
Copy link
Contributor Author

satorg commented May 18, 2020

BTW, I realized that there are some cases which cannot be fixes properly. For example, consider the following:

trait MyIntermediateService[F[_]] {
  def client: Client[F[_]]

  final def someFuncThatCallsClientFetch[A](req: Request[F]): F[A] = {
    client.fetch(req) { res => /* do something */ }
  }
}

The problem with this sample is that Client.fetch itself doesn't require any typeclasses, but its replacement (Client.run followed by Resource.use) requires a Bracket. But since the enclosing trait doesn't have a Bracket for F[_], the resulting code just won't compile. And I doubt if it is really possible to automate it somehow...

@SystemFw
Copy link
Member

SystemFw commented May 18, 2020

ah, that's a great point. It would be nice if we could somehow inject a custom error for that somewhere

@satorg
Copy link
Contributor Author

satorg commented May 18, 2020

ah, that's a great point. It would be nice if we could somehow inject a custom error for that somewhere

Hmm... any ideas on that? As an alternative we could try to add a TODO comment to point out at the problematic line.

@satorg satorg force-pushed the deprecate-client-fetch branch 2 times, most recently from 0133a66 to 4849b27 Compare May 23, 2020
@satorg
Copy link
Contributor Author

satorg commented May 23, 2020

@rossabaker I managed to run the new rule on the project sources in this way:

  1. From the scalafix project publish the "rules" module locally (it gets published as org.http4s:scalafix:0.1.0-SNAPSHOT).
  2. Temporarily add the sbt-scalafix plugin to http4s project.
  3. Enable it with scalafixEnable
  4. Apply the rule on main and test sources (on master branch) by executing compile:scalafix and test:scalafix with dependency:v0_21@org.http4s:scalafix:0.1.0-SNAPSHOT arg.

@@ -30,6 +30,7 @@ trait Client[F[_]] {
* response body afterward will result in an error.
* @return The result of applying f to the response to req
*/
@deprecated("Use run(req).use(f)", "0.21.5")
Copy link
Contributor

@kevinmeredith kevinmeredith May 24, 2020

Choose a reason for hiding this comment

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

I question whether this message should indicate, "Don't use fetch, but, if you must, do run(req).use(f) per the helpful discussion in #3345 (comment) regarding what's wrong with fetch.

Thoughts, @rossabaker and @satorg?

I admit that I don't know if "why is it deprecated" should be placed inside of the @deprecated's message.

Copy link
Contributor Author

@satorg satorg May 30, 2020

Choose a reason for hiding this comment

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

Sorry, didn't get, what is your concern about the message? Do you think it should be more descriptive?

Copy link
Member

@rossabaker rossabaker May 31, 2020

Choose a reason for hiding this comment

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

I think the message is in line with what we usually do.

Copy link
Contributor

@kevinmeredith kevinmeredith Jun 1, 2020

Choose a reason for hiding this comment

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

Thanks for addressing my question, satorg and Ross.

@satorg satorg force-pushed the deprecate-client-fetch branch 4 times, most recently from bd4d12f to ff4ad72 Compare May 30, 2020
@satorg satorg marked this pull request as ready for review May 30, 2020
@satorg satorg force-pushed the deprecate-client-fetch branch from 0213edf to d2ec37d Compare Jun 23, 2020
@rossabaker
Copy link
Member

rossabaker commented Jun 24, 2020

Oh, are we targeting 0.21.5 with this? I thought we were, and the deprecation warnings say so, but I just noticed that it's on master.

@satorg
Copy link
Contributor Author

satorg commented Jun 24, 2020

Yes, it is.. It was on 'master' from the beginning.. As I've realized now, I should rebase it onto '0.21.5'... I'll do it later today, sorry for misunderstanding.

@rossabaker rossabaker added the retarget label Jun 24, 2020
@rossabaker
Copy link
Member

rossabaker commented Jun 24, 2020

Okay, I've rebased this one onto 0.21 so it's part of that release.

Thanks, @satorg, for the excellent work on both the deprecation and improving our scalafix situation. This was a long ticket, but well worth it.

@rossabaker rossabaker closed this Jun 24, 2020
@fthomas
Copy link
Contributor

fthomas commented Jun 25, 2020

I just learned about the new Scalafix migrations via the release notes of 0.21.5. I think it would be nice to add the new migration to Scala Steward although it already created a lot of PRs for http4s 0.21.5 without it. For future releases it would be nice to add new migrations to Scala Steward before a new version is released to ensure that Scala Steward can run them on all open source projects it is working on.

@rossabaker
Copy link
Member

rossabaker commented Jun 25, 2020

Oh, I knew that scalafix integration had been discussed, but I didn't realize it was a reality. Very cool!

@satorg satorg deleted the deprecate-client-fetch branch Jun 25, 2020
@fthomas
Copy link
Contributor

fthomas commented Jun 26, 2020

The rule for http4s 0.20.0 was even one of the first migrations added to Scala Steward and was used in 30+ PRs although it was also added after the 0.20.0 release.

@rossabaker
Copy link
Member

rossabaker commented Jun 26, 2020

Would the right thing to do be to add the new rule now, then?

@rossabaker
Copy link
Member

rossabaker commented Jun 26, 2020

Oh, I should have looked at your linked PR.

@fthomas
Copy link
Contributor

fthomas commented Jun 29, 2020

I've added the new migration in scala-steward-org/scala-steward#1510. It will be applied to updates going from x < 0.21.5 to y >= 0.21.5. For future migrations, you can ping me when they are discussed/merged and I can help with adding them to Scala Steward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
retarget
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants