fs2 support #1042

sergeykolbasov · 2018-11-18T17:01:52Z

Related to #1001

Idea is to have streamBody and jsonStreamBody endpoints in core, along with StreamingLaws, so modules like iteratee and fs2 could implement only neccessary type classes and couple of specs while the rest of machinery is done inside of the core.

Refactoring
fs2 module + circe instances for it

codecov-io · 2018-11-18T22:17:56Z

Codecov Report

Merging #1042 into master will increase coverage by 0.15%.
The diff coverage is 80.7%.

@@            Coverage Diff            @@
##           master   #1042      +/-   ##
=========================================
+ Coverage   83.95%   84.1%   +0.15%     
=========================================
  Files          50      53       +3     
  Lines         941     969      +28     
  Branches       56      57       +1     
=========================================
+ Hits          790     815      +25     
- Misses        151     154       +3

Impacted Files	Coverage Δ
...re/src/main/scala/io/finch/internal/ToEffect.scala	`88.88% <ø> (ø)`	⬆️
...src/main/scala/io/finch/EndpointStreamModule.scala	`0% <0%> (ø)`
...amples/src/main/scala/io/finch/iteratee/Main.scala	`0% <0%> (ø)`	⬆️
circe/src/main/scala/io/finch/circe/Decoders.scala	`100% <100%> (ø)`	⬆️
...in/scala/io/finch/circe/AccumulatingDecoders.scala	`100% <100%> (ø)`	⬆️
...c/main/scala/io/finch/streaming/DecodeStream.scala	`100% <100%> (ø)`
...ore/src/main/scala/io/finch/internal/package.scala	`97.5% <100%> (+0.06%)`	⬆️
...in/scala/io/finch/streaming/StreamFromReader.scala	`100% <100%> (ø)`
core/src/main/scala/io/finch/Endpoint.scala	`71.59% <70%> (-1.31%)`	⬇️
...tee/src/main/scala/io/finch/iteratee/package.scala	`92.3% <85.71%> (+0.64%)`	⬆️
... and 7 more

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 49ec71b...2cceff1. Read the comment docs.

vkostyukov · 2018-11-19T19:27:50Z

Hey @sergeykolbasov! I see this is WIP - let me know whey you think this is ready for review.

sergeykolbasov · 2018-11-19T22:25:29Z

@vkostyukov It's good to go

vkostyukov

I only looked at the big picture. As usual, this is an amazing work @sergeykolbasov!

My big questions are:

Can we get rid of StreamFromReader
Can we provide missing instances to make streamBody[Reader, ???, Buf] work (would really like that; in future this could be available via an alias streamBodyReader[???, Buf]).
Can we provide missing instances to make streamBody[AsyncStream, ???, Buf] work (less concerned about that - want to rip off any AsyncStream bits from Finch in the long run, Reader is fine though).

build.sbt

core/src/main/scala/io/finch/internal/package.scala

core/src/main/scala/io/finch/streaming/StreamDecode.scala

core/src/main/scala/io/finch/streaming/StreamFromReader.scala

core/src/main/scala/io/finch/EndpointModule.scala

vkostyukov

More comments from my side. I still need to think about wether we need to merge those type-classes together.

iteratee/src/main/scala/io/finch/iteratee/package.scala

fs2/src/main/scala/io/finch/fs2/package.scala

core/src/main/scala/io/finch/EndpointModule.scala

vkostyukov · 2018-12-14T22:46:47Z

Hey @sergeykolbasov! I was thinking we could ship this in 0.27, before the NY. What's your plan with this?

sergeykolbasov · 2018-12-15T10:29:32Z

I'm in the process of polishing things

sergeykolbasov · 2018-12-16T16:45:41Z

@vkostyukov it should be fine by now
Also, I'll be out for the upcoming week.

vkostyukov

Looks just great @sergeykolbasov! Just some nits from me. I feel it's pretty much ready to. The biggest concern I have is about backwards compatibility.

core/src/main/scala/io/finch/streaming/StreamFromReader.scala

iteratee/src/main/scala/io/finch/iteratee/package.scala

vkostyukov · 2018-12-17T19:17:30Z

core/src/main/scala/io/finch/EndpointResult.scala

@@ -45,6 +45,11 @@ sealed abstract class EndpointResult[F[_], +A] {
    case _ => None
  }

+  final def map[B](fn: A => B)(implicit F: Effect[F]): EndpointResult[F, B] = this match {


Why this is needed?

I tried to make streamJsonBody simpler. We still need the access to Input.request.charsetOrUtf8 to run the decoder, so it's impossible to just use streamBody.map(a => b).
Check implementation of this endpoint.

core/src/main/scala/io/finch/EndpointStreamModule.scala

core/src/main/scala/io/finch/internal/package.scala

core/src/main/scala/io/finch/streaming/StreamFromReader.scala

vkostyukov

Did one more run which raised more questions - nothing major but we should try to address them.

circe/src/main/scala/io/finch/circe/Decoders.scala

fs2/src/main/scala/io/finch/fs2/package.scala

sergeykolbasov · 2019-01-03T23:33:48Z

@vkostyukov mind to give it another look?

Let's make this PR a New Year resolution 😄

vkostyukov · 2019-01-03T23:53:49Z

Going to look tonight @sergeykolbasov!

What's your opinion on this vs #1056. Which one should go first?

vkostyukov

I like all of this! Just couple of nits and this is ready to go!

core/src/main/scala/io/finch/streaming/DecodeStream.scala

core/src/main/scala/io/finch/internal/ToEffect.scala

Remove futureToEffect in favor of F.defer Remove EndpointStreamModule Make ToEffect call-by-value again

sergeykolbasov · 2019-01-04T14:26:20Z

That's creepy. I can't reproduce the build error on my machine. Assume, there is a conflict in circe-jawn between 0.11.0 and 0.10.0 as circe-fs2 still depends on the old one.

travisbrown · 2019-01-04T14:52:51Z

@sergeykolbasov Hoping to have circe-fs2 published soon—its tests turned up a bug in Jawn 0.14.0 that I thought we could merged pretty quickly: typelevel/jawn#144

sergeykolbasov · 2019-01-04T14:55:17Z

Thanks for the info, Travis!

Waiting for the resolution then

vkostyukov

@travisbrown Thanks for the details!

@sergeykolbasov I'm happy when CI is happy! Appreciate your hard work on this. When it's merged, I can rebase #1056 so we can take another look at it.

sergeykolbasov force-pushed the fs2-support branch 3 times, most recently from b6fc90c to 5869a46 Compare November 18, 2018 17:13

sergeykolbasov changed the title ~~[WIP] fs2 support~~ fs2 support Nov 19, 2018

sergeykolbasov requested review from vkostyukov, rpless and travisbrown November 19, 2018 22:25

vkostyukov reviewed Nov 20, 2018

View reviewed changes

vkostyukov reviewed Nov 28, 2018

View reviewed changes

iteratee/src/main/scala/io/finch/iteratee/package.scala Show resolved Hide resolved

fs2/src/main/scala/io/finch/fs2/package.scala Show resolved Hide resolved

core/src/main/scala/io/finch/EndpointModule.scala Outdated Show resolved Hide resolved

vkostyukov mentioned this pull request Nov 28, 2018

add support for FunctionK #1044

Closed

vkostyukov mentioned this pull request Dec 5, 2018

mapper for F[_] should be generic at kind level #1045

Merged

sergeykolbasov force-pushed the fs2-support branch from 9e86108 to 94bf793 Compare December 5, 2018 23:25

vkostyukov mentioned this pull request Dec 10, 2018

Input should be able to create chunked requests #1038

Closed

sergeykolbasov force-pushed the fs2-support branch 3 times, most recently from 485777c to 39bf504 Compare December 15, 2018 10:20

vkostyukov reviewed Dec 17, 2018

View reviewed changes

vkostyukov mentioned this pull request Dec 21, 2018

Abstract over Stream type in the outbound path #1056

Merged

sergeykolbasov force-pushed the fs2-support branch from 13b9380 to 93eb837 Compare January 3, 2019 23:21

vkostyukov requested changes Jan 4, 2019

View reviewed changes

core/src/main/scala/io/finch/streaming/DecodeStream.scala Outdated Show resolved Hide resolved

core/src/main/scala/io/finch/internal/ToEffect.scala Outdated Show resolved Hide resolved

Refactor streaming

1f156ab

sergeykolbasov added 18 commits January 4, 2019 15:50

Add fs2 module

75ad574

Add circe-fs2 library and stream decoder instance

f8e716c

Fix example

5119271

Add AsyncStreamF type alias and LiftToEffect type class

c7c6ba5

Remove dependencies from circe module

98c5d5f

Add EndpointStreamModule and move aliases there

a20c6f5

Rename StreamDecode -> DecodeStream and fix implicitNotFound msg

9d2be53

Lift futures into Eval.later before turning into Effect

4c7e2e3

Fix comment

eceb4fb

Change ToEffect.apply argument to be call-by-name

a69b9e2

Add EndpointResult.map and rework streamBody to streamBinaryBody

3fd2617

Add Functor instance for EndpointResult and corresponding spec

8b2a0d0

Put enumeratorBody back

351ccef

Remove annoying warning

27a1f66

Remove AsyncStreamF

30f6a37

Remove futureToEffect in favor of F.defer Remove EndpointStreamModule Make ToEffect call-by-value again

Rename StreamFromReader to LiftReader

77d3dc9

Transform fs2.Chunk[Buf] to fs2.Chunk[Byte]

eda232b

Fix ToEffectLaws to remove deferred law

b003487

sergeykolbasov force-pushed the fs2-support branch from e4c4525 to 4c5b350 Compare January 4, 2019 13:52

Move DecodeStream and LiftReader to io.finch package

cb3f907

sergeykolbasov force-pushed the fs2-support branch from 4c5b350 to cb3f907 Compare January 4, 2019 13:57

vkostyukov approved these changes Jan 4, 2019

View reviewed changes

Bump circe-fs2 version

2cceff1

sergeykolbasov merged commit 66f15e2 into finagle:master Jan 8, 2019

vkostyukov mentioned this pull request Jan 9, 2019

Support to fs2 #1001

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs2 support #1042

fs2 support #1042

sergeykolbasov commented Nov 18, 2018 •

edited

codecov-io commented Nov 18, 2018 •

edited

vkostyukov commented Nov 19, 2018

sergeykolbasov commented Nov 19, 2018

vkostyukov left a comment •

edited

vkostyukov left a comment

vkostyukov commented Dec 14, 2018

sergeykolbasov commented Dec 15, 2018

sergeykolbasov commented Dec 16, 2018 •

edited

vkostyukov left a comment

vkostyukov Dec 17, 2018

sergeykolbasov Dec 21, 2018

vkostyukov left a comment

sergeykolbasov commented Jan 3, 2019

vkostyukov commented Jan 3, 2019

vkostyukov left a comment

sergeykolbasov commented Jan 4, 2019

travisbrown commented Jan 4, 2019 •

edited

sergeykolbasov commented Jan 4, 2019

vkostyukov left a comment

fs2 support #1042

fs2 support #1042

Conversation

sergeykolbasov commented Nov 18, 2018 • edited

codecov-io commented Nov 18, 2018 • edited

Codecov Report

vkostyukov commented Nov 19, 2018

sergeykolbasov commented Nov 19, 2018

vkostyukov left a comment • edited

Choose a reason for hiding this comment

vkostyukov left a comment

Choose a reason for hiding this comment

vkostyukov commented Dec 14, 2018

sergeykolbasov commented Dec 15, 2018

sergeykolbasov commented Dec 16, 2018 • edited

vkostyukov left a comment

Choose a reason for hiding this comment

vkostyukov Dec 17, 2018

Choose a reason for hiding this comment

sergeykolbasov Dec 21, 2018

Choose a reason for hiding this comment

vkostyukov left a comment

Choose a reason for hiding this comment

sergeykolbasov commented Jan 3, 2019

vkostyukov commented Jan 3, 2019

vkostyukov left a comment

Choose a reason for hiding this comment

sergeykolbasov commented Jan 4, 2019

travisbrown commented Jan 4, 2019 • edited

sergeykolbasov commented Jan 4, 2019

vkostyukov left a comment

Choose a reason for hiding this comment

sergeykolbasov commented Nov 18, 2018 •

edited

codecov-io commented Nov 18, 2018 •

edited

vkostyukov left a comment •

edited

sergeykolbasov commented Dec 16, 2018 •

edited

travisbrown commented Jan 4, 2019 •

edited