Implement case class default parameters #42

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants

opyate commented Nov 29, 2011

(as discussed via email)

You have your domain stuff as case classes with default parameters,
e.g. case class Message(msg: String, priority: String = "low")
So, Message(msg="hey") will have a default low priority.

Now you can send something like this on the messaging infrastructure:

{
"msg" : "Hello, World!"
}

...and the serializer in your message converter won't complain about
the lack of priority flag, and will serialize a Message with "low"
priority.

The patch is very useful, thanks. The only correction is that the default values should be evaluated on each invokation, not just once as in the current implementation. The difference is visible if default arguments have side effects:

case class C(id: Int, createdOn: Date = new Date)

Each time you invoke C(1), you will have the current date stored in createdOn field. However, each time you invoke Json.parse[C]("""{"id":1}""") you'll get the same date in every object. To fix it, just store closure instead of a pre-computed value when parsing class signatures:

val defaultParam = defaultMethod.map(m => () => m.invoke(companionObject))

and invoke it when creating the object:

if (field != null || value != null) {
  values += value
} else {
  // see if a default value was supplied
  paramDefault.foreach ( values += _() )
}
@ghost

ghost commented Apr 1, 2012

@ ksvladimir Good catch!

kbarros commented Jun 22, 2012

I've been really wanting this feature too. Any chance it could get merged into codahale:development ?

@artgon artgon added a commit to artgon/jerkson that referenced this pull request Jul 7, 2012

@artgon artgon customized deploy, added pull request #42 178e78d

artgon commented Aug 7, 2012

Thanks to @opyate and @ksvladimir for the update to the original patch. I have implemented it in my fork.

@artgon artgon added a commit to artgon/jerkson that referenced this pull request Aug 7, 2012

@artgon artgon updated original fix from pull request #42 4b24b52

kul commented Aug 22, 2012

+1 please pull this.

efuquen commented Aug 30, 2012

It seems there has been a lot of pull requests recently that are basically just sitting in limbo. Is this project no longer actively maintained?

+1 very useful addition, would love to see this merged.

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