Skip to content

Conversation

@wsong
Copy link

@wsong wsong commented Sep 22, 2015

Fixes issue #165 .

Copy link
Member

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

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

I like the general idea, I think the API could be improved a bit and documentation is still missing.

jsonFormat(construct, [#p1#])
}
def jsonFormat[[#P1 :JF#], T <: Product](construct: ([#P1#]) => T, [#fieldName1: String#]): RootJsonFormat[T] = new RootJsonFormat[T]{
val knownFields = HashSet[String]()
Copy link
Member

Choose a reason for hiding this comment

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

Can we try to make the feature free when allowExtraKeys is false (default behavior). Maybe just make this a lazy val to make sure it is not initialized when not needed.

val keySet = jsObject.fields.keys.toSet
val keySetDiff = keySet.diff(knownFields)
if (!keySetDiff.isEmpty) {
throw new DeserializationException(s"${keySetDiff.head} is not a known key", null, keySetDiff.toList)
Copy link
Member

Choose a reason for hiding this comment

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

Can we give more complete information when it fails? Missing set, given set and expected set of keys.

* will throw an error if the input contains keys that are not a field of the
* target case class.
*/
trait ExtraKeysOptions extends ProductFormats {
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need that trait? Seems a bit weird API to just change the flag from true to false. Maybe just document allowExtraKeys directly.

import scala.collection.mutable.HashSet

trait ProductFormatsInstances { self: ProductFormats with StandardFormats =>
def allowExtraKeys: Boolean = true
Copy link
Member

Choose a reason for hiding this comment

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

I wonder (but I'm not sure) if reversing the naming could be more clear? failOnExtraKeys or something like this?

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.

2 participants