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

Fix ensure #1025

Merged
merged 4 commits into from Dec 18, 2018
Merged

Fix ensure #1025

merged 4 commits into from Dec 18, 2018

Conversation

travisbrown
Copy link
Member

In the [0.10.0] release we introduced a new Decoder#ensure method that was semantically kind of weird and included some behavior that (when used together with error accumulation) was definitely buggy:

scala> import io.circe.{ Decoder, Json }
import io.circe.{Decoder, Json}

scala> val decodePos: Decoder[Int] = Decoder[Int].ensure(_ > 0, "not positive")
decodePos: io.circe.Decoder[Int] = io.circe.Decoder$$anon$25@5b9070e3

scala> decodePos.accumulating(Json.Null.hcursor)
res0: io.circe.AccumulatingDecoder.Result[Int] = Invalid(NonEmptyList(DecodingFailure(Int, List()), DecodingFailure(not positive, List())))

Thanks to @ylaurent for noticing and reporting this.

The tests would have caught this bug, but the relevant test was itself buggy, containing a check for oddness that said e.g. -1 was not odd.

I've changed the documentation for the previous version of ensure to state explicitly that dec.ensure(p1, "p1").ensure(p2, "p2") will only provide the first failure, and have added a new, more powerful ensure method that allows the user to provide multiple errors that depend on the decoded value.

@pfcoperez, I was thinking we might be able to add a similar HCursor => List[String] variant for validate that would accommodate the use case supported by your #1020, and that would keep these two methods consistent. What do you think?

@pfcoperez
Copy link
Contributor

pfcoperez commented Dec 4, 2018

@pfcoperez, I was thinking we might be able to add a similar HCursor => List[String] variant for validate that would accommodate the use case supported by your #1020, and that would keep these two methods consistent. What do you think?

@travisbrown I think it is a great idea! I'll update my PR with that variant. That is, I'll replace the HCursor => Boolean, HCursor => String with HCursor => List[String] and implement the legacy method def validate(pred: HCursor => Boolean, message: => String) with it.

@travisbrown
Copy link
Member Author

Thanks, @pfcoperez!

@codecov-io
Copy link

codecov-io commented Dec 18, 2018

Codecov Report

Merging #1025 into master will decrease coverage by 0.05%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1025      +/-   ##
==========================================
- Coverage   85.39%   85.34%   -0.06%     
==========================================
  Files          75       75              
  Lines        2383     2388       +5     
  Branches      168      170       +2     
==========================================
+ Hits         2035     2038       +3     
- Misses        348      350       +2
Impacted Files Coverage Δ
.../core/shared/src/main/scala/io/circe/Decoder.scala 92.37% <66.66%> (-0.82%) ⬇️
...re/shared/src/main/scala/io/circe/JsonNumber.scala 91.13% <0%> (-1.27%) ⬇️
...rc/main/scala/io/circe/numbers/BiggerDecimal.scala 90.96% <0%> (+0.6%) ⬆️
...les/core/shared/src/main/scala/io/circe/Json.scala 79.61% <0%> (+0.63%) ⬆️

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 ff2c4b0...1c5ab5d. Read the comment docs.

@travisbrown travisbrown merged commit 708f732 into master Dec 18, 2018
@travisbrown travisbrown deleted the topic/fix-ensure branch June 11, 2019 07:36
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.

None yet

3 participants