-
Notifications
You must be signed in to change notification settings - Fork 247
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
scrooge/finagle: Remove generated HKT, FutureIface and their friends
Problem FutureIface is deprecated. Scrooge is more powerful when used with finagle integration, and in that case, higher-kinded-types won't play its advantages. Scrooge generated code is growing which increases source compilation time, we should get rid of the unused parts. Solution Remove them from code-gen. In generated code, replace HKT/FutureIface with MethodPerEndpoint. Remove some deprecated builders in both scrooge-gen and finagle that require HKT as the parameter. Differential Revision: https://phabricator.twitter.biz/D747744
- Loading branch information
Showing
15 changed files
with
150 additions
and
258 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 changes: 8 additions & 5 deletions
13
scrooge-generator-tests/src/test/scala/com/twitter/scrooge/backend/NamespaceSpec.scala
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,22 @@ | ||
| package com.twitter.scrooge.backend | ||
|
|
||
| import com.twitter.conversions.DurationOps._ | ||
| import com.twitter.scrooge.testutil.Spec | ||
| import com.twitter.util.Await | ||
| import com.twitter.util.Future | ||
|
|
||
| class NamespaceSpec extends Spec { | ||
| "Scala Generator" should { | ||
| import bar._ | ||
| import com.fake._ | ||
| "import from another namespace" in { | ||
| val service: Restaurant[Some] = new Restaurant[Some] { | ||
| def isOpen(whichDay: Weekday) = Some(whichDay != Weekday.Monday) | ||
| val service: Restaurant.MethodPerEndpoint = new Restaurant.MethodPerEndpoint { | ||
| def isOpen(whichDay: Weekday) = Future.value(whichDay != Weekday.Monday) | ||
| def makeReservation(whichDay: Weekday, howMany: Int) = | ||
| Some(if (whichDay == Weekday.Monday) 0 else howMany) | ||
| Future.value(if (whichDay == Weekday.Monday) 0 else howMany) | ||
| } | ||
| service.makeReservation(Weekday.Monday, 2) must be(Some(0)) | ||
| service.makeReservation(Weekday.Tuesday, 2) must be(Some(2)) | ||
| Await.result(service.makeReservation(Weekday.Monday, 2), 5.seconds) must equal(0) | ||
| Await.result(service.makeReservation(Weekday.Tuesday, 2), 5.seconds) must equal(2) | ||
| } | ||
| } | ||
| } |
44 changes: 0 additions & 44 deletions
44
scrooge-generator-tests/src/test/scala/com/twitter/scrooge/backend/NonFinagleSpec.scala
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
8d768caThere 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'm sad to see HKTs go.
Twitter futures are not an ideal interface for writing code in a functional style, and the HKT served as an easy way to switch that interface.
There is even a library for it.
I do not look forward to the boilerplate that'll be required if we need to upgrade.
Perhaps this use-case is a vocal minority, and others appreciate the more compact generated code.
Nevertheless I think it is worth reconsidering dropping the HKT's.
8d768caThere 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.
@henryxparker thanks for the feedback! We actually didn't realize that anyone was using this interface. I had never seen the dwolla library before. As far as we knew it was an unused API (we have never used it internally).
Can you tell me more about how you use Scrooge? Do you use Scrooge + Finagle under the hood, but convert all of the futures to Scala Futures with a bijection?
Also, would you mind filing an issue related to this? I'm not sure if we'll reintroduce HKT into the normal Scrooge generated code, but we may be able to continue generating it as a plugin.
I'm sorry for breaking your use case, but I'm optimistic that we can work out a compromise that will work for you.
8d768caThere 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.
@mossen thank you so much for the response! I do indeed use scrooge with finagle.
And it's not exactly a bijection. I have access to a
twitterFuture.asScalaextension method and vice-versa that can take care of that conversion with relative ease, so that's not the primary reason I use the HKT interface.I use the HKT interface because I started using
cats.effect.IO. The reasoning being the same as in this article.Since
Futures start executing as soon as they are defined, I could no longer use thetwitterFuture.asIOextension method syntax if I wanted to keep the advantages ofIO.That's why the dwolla library exists. It provides a way to automatically convert a
Service[Future]to aService[IO]safely.I've added the issue #352 as requested. Thanks again for listening!