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

Handle Option[] parameters with no default. #31

Merged
merged 3 commits into from
Mar 10, 2019

Conversation

adampauls
Copy link
Contributor

Circe permits case class parameters of type Option[T] with no default specified to still be decoded (to None) when the parameter is missing. This PR brings circe-magnolia in line with this behavior.

@codecov-io
Copy link

codecov-io commented Mar 2, 2019

Codecov Report

Merging #31 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #31   +/-   ##
=======================================
  Coverage   99.16%   99.16%           
=======================================
  Files          11       11           
  Lines         120      120           
  Branches       10        9    -1     
=======================================
  Hits          119      119           
  Misses          1        1
Impacted Files Coverage Δ
...main/scala/io/circe/magnolia/MagnoliaDecoder.scala 98.38% <100%> (ø) ⬆️

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 8cd0666...da16f9b. Read the comment docs.

@@ -34,7 +34,11 @@ private[magnolia] object MagnoliaDecoder {
val keyCursor = c.downField(key)
keyCursor.focus match {
case Some(json) => json.as[p.PType](p.typeclass)
case None => p.default.toRight(DecodingFailure("Attempt to decode value on failed cursor", keyCursor.history))
case None => p.default.map { default => Right(default) }.getOrElse {
Copy link
Owner

Choose a reason for hiding this comment

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

good, I would just use fold:

p.default.fold(p.typeclass.tryDecode(keyCursor))(Right(_))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I've never liked Option.fold because it puts the None case first, which is not natural to me, but happy to make the change.

@adampauls
Copy link
Contributor Author

RFAL, thanks!

@vpavkin
Copy link
Owner

vpavkin commented Mar 10, 2019

Thanks, @adampauls !

@vpavkin vpavkin merged commit 1d12a6d into vpavkin:master Mar 10, 2019
@vpavkin
Copy link
Owner

vpavkin commented Dec 9, 2019

@adampauls Pushed 0.5.0 release with this change.
Will go through scala-steward updates next and do another one

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.

3 participants