Skip to content

Aggregate refined#969

Merged
vkostyukov merged 2 commits into
finagle:masterfrom
rider-yi:fix-refined
Jul 25, 2018
Merged

Aggregate refined#969
vkostyukov merged 2 commits into
finagle:masterfrom
rider-yi:fix-refined

Conversation

@rider-yi
Copy link
Copy Markdown
Contributor

@rider-yi rider-yi commented Jul 25, 2018

I just noticed that the new finch-refined module wasn't being published.

[warn] 	::::::::::::::::::::::::::::::::::::::::::::::
[warn] 	::          UNRESOLVED DEPENDENCIES         ::
[warn] 	::::::::::::::::::::::::::::::::::::::::::::::
[warn] 	:: com.github.finagle#finch-refined_2.12;0.22.0: not found
[warn] 	::::::::::::::::::::::::::::::::::::::::::::::

@vkostyukov
Copy link
Copy Markdown
Collaborator

Oh that's a good catch. Thanks @rider-yi!

CI fails with the following error on 2.11:

/home/travis/build/finagle/finch/refined/src/main/scala/io/finch/refined/package.scala:12:88: value toOption is not a member of Either[String,F[A,B]]
[error]   ): DecodePath[F[A, B]] = DecodePath.instance(s => ad(s).flatMap(p => rt.refine[B](p).toOption))

Do you have any idea why? I restarted the job just in case.

@rpless
Copy link
Copy Markdown
Collaborator

rpless commented Jul 25, 2018

@vkostyukov I think its Scala 2.11 v 2.12 related. Pretty sure Either doesn't have toOption in Scala 2.11. Cats has an implicit syntax for it. We'll either have to use that or manually fold the Either here.

@vkostyukov
Copy link
Copy Markdown
Collaborator

I see. Sounds like we never compiled it for 2.11 hence never saw an error. Importing Cats' syntax works for me, fwiw. @rider-yi any chance you want to fix this as part of this PR?

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 25, 2018

Codecov Report

Merging #969 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #969      +/-   ##
==========================================
+ Coverage   83.47%   83.57%   +0.09%     
==========================================
  Files          50       52       +2     
  Lines         835      840       +5     
  Branches       44       44              
==========================================
+ Hits          697      702       +5     
  Misses        138      138
Impacted Files Coverage Δ
...ined/src/main/scala/io/finch/refined/package.scala 100% <100%> (ø)
.../main/scala/io/finch/refined/PredicateFailed.scala 0% <0%> (ø)
core/src/main/scala/io/finch/Error.scala 72.72% <0%> (+9.09%) ⬆️

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 5746531...c501c06. Read the comment docs.

@rider-yi
Copy link
Copy Markdown
Contributor Author

I just fixed the error with RightProjection. It seems that importing Cats's syntax causes unused warning:

[warn] /Users/yu_ishikawa/finch/refined/src/main/scala/io/finch/refined/package.scala:3:27: Unused import
[warn] import cats.syntax.either._
[warn]                           ^

@vkostyukov
Copy link
Copy Markdown
Collaborator

Changes look good to me! Thanks again, @rider-yi!

@vkostyukov vkostyukov merged commit 11d20f0 into finagle:master Jul 25, 2018
@vkostyukov
Copy link
Copy Markdown
Collaborator

Cherry-picked this into arrows: 3ffa880 and bd92d61.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants