8000 [generic-extra] Decode default value on Option if key is missing. by exoego · Pull Request #994 · circe/circe · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[generic-extra] Decode default value on Option if key is missing. #994

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 18, 2018
Merged

[generic-extra] Decode default value on Option if key is missing. #994

merged 1 commit into from
Dec 18, 2018

Conversation

exoego
Copy link
Contributor
@exoego exoego commented Oct 20, 2018 •
edited
Loading

Closes #878

without default
Foo(a: Option[Int])		 with default
Foo(a: Option[Int]=Some(0))
null given
{"a":null}		 Foo(None) Foo(None)
key missing
{}		 Foo(None) right now: Foo(None)
This PR: Foo(Some(0))

@exoego exoego changed the title Use default value on Option if key is missing. [WIP] Use default value on Option if key is missing. Oct 20, 2018
@exoego exoego force-pushed the default-if-key-missing branch from fc477a0 to 1c37551 Compare October 20, 2018 04:25
@codecov-io
Copy link
codecov-io commented Oct 20, 2018 •
edited
Loading

Codecov Report

Merging #994 into master will decrease coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #994      +/-   ##
==========================================
- Coverage   85.49%   85.21%   -0.29%     
==========================================
  Files          75       75              
  Lines        2386     2388       +2     
  Branches      174      181       +7     
==========================================
- Hits         2040     2035       -5     
- Misses        346      353       +7
Impacted Files Coverage Δ
.../core/shared/src/main/scala/io/circe/Decoder.scala 92.92% <100%> (+0.02%) ⬆️
...io/circe/generic/extras/decoding/ReprDecoder.scala 69.56% <100%> (+1.38%) ⬆️
...re/shared/src/main/scala/io/circe/JsonNumber.scala 91.13% <0%> (-3.8%) ⬇️
...rc/main/scala/io/circe/numbers/BiggerDecimal.scala 88.55% <0%> (-2.41%) ⬇️

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 686bb10...e3758c6. Read the comment docs.

@exoego exoego force-pushed the default-if-key-missing branch from 1c37551 to 28d16b7 Compare October 20, 2018 04:30
@exoego exoego changed the title [WIP] Use default value on Option if key is missing. [generic-extra] Use default value on Option if key is missing. Oct 20, 2018
@exoego exoego changed the title [generic-extra] Use default value on Option if key is missing. [generic-extra] Decode default value on Option if key is missing. Oct 20, 2018
@exoego exoego force-pushed the default-if-key-missing branch 2 times, most recently from d9d3bb9 to 7d33d8e Compare October 21, 2018 00:59
travisbrown
travisbrown reviewed Oct 21, 2018
View reviewed changes
modules/generic-extras/src/main/scala/io/circe/generic/extras/decoding/ReprDecoder.scala
@@ -34,10 +34,10 @@ abstract class ReprDecoder[A] extends Decoder[A] {
name: String,
defaults: Map[String, Any]
): Decoder.Result[B] = result match {
case r @ Right(_) => r
case l @ Left(_) => defaults.get(name) match {
case r @ Right(_) if r ne Decoder.keyMissingNone => r
Copy link
Member

Choose a reason for hiding this comment

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

Why not just case r @ Right(v) if v.isEmpty => r?

Copy link
Contributor Author
@exoego exoego Oct 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This trick is required to discriminate null-given case from key-missing case.
If null given, it is dedoded to Decoder.rightNone which is ne Decoder.keyMissingNone, so Right(None) will be returned.
Otherwise, it tests there is default or not, and return the found default value in case of Decoder.keyMissingNone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@travisbrown
Can I have your moment to get review?
Perhaps, you may missed my response due to last GitHub outage.
Thanks in advance 😃

@exoego exoego force-pushed the default-if-key-missing branch 3 times, most recently from b9a371e to 0d0f2d0 Compare November 13, 2018 09:14
@exoego exoego force-pushed the default-if-key-missing branch from 0d0f2d0 to e3758c6 Compare November 13, 2018 09:32
@exoego exoego mentioned this pull request Nov 29, 2018
Closed
@travisbrown
Copy link
Member

Thanks much, @exoego—sorry for the long delay in review, but this looks reasonable to me, and will be released in 0.11.0.

@travisbrown travisbrown merged commit e3c962c into circe:master Dec 18, 2018
@niij
Copy link
niij commented Dec 18, 2018

@exoego Thank you for this!

travisbrown added a commit that referenced this pull request Dec 18, 2018
@travisbrown
Same fix as #994 but for error-accumulating decoding
b5c8b8e
@travisbrown travisbrown mentioned this pull request Dec 18, 2018
Merged
travisbrown added a commit that referenced this pull request Dec 18, 2018
@travisbrown
Same fix as #994 but for error-accumulating decoding
09de915
@exoego exoego deleted the default-if-key-missing branch December 18, 2018 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@travisbrown travisbrown travisbrown left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@exoego @codecov-io @travisbrown @niij
0