Adding date field

[ppamorim]

Implementing this the developer is able to parse the date using a lambda function. I was think to implement something like this:

val obj = parse("/object.json") as JsonObject
val date: Date? = obj.date("date", { Iso8601Utils.parse(it) })

We can also provide another artifact just to resolve the Iso8601Utils dependency like other libraries used to do. What you think about it?

Thank you.

[cbeust]

I think you can simplify as follows:

= (get(fieldName) as? String?).apply { formatter(value) }

(I haven't tested it, just writing it off the top of my head)

[cbeust]

Can you explain what you mean by "pruned to Moshi's needs"? What does Moshi have to do with this?

[ppamorim]

Removing it. :)

[damphat]

Questions:

• In this version, Klaxon does not support JSON.stringify(date), why would we support date parser?

import com.beust.klaxon.valueToString
var json = valueToString(listOf(Date()))
print(json) // "[ Thu Aug 17 03:19:05 PDT 2017 ]"

• ISO8601Utils.java is created for GC free, so it is not suitable for this project. Why not replace with native date parser of java?

[ppamorim]

@cbeust I can remove this component if needed. I just added it to be used on the unit test. This code comes from Moshi.

[ppamorim]

@damphat In my opnion the parse and stringify of the date needs to be done by a external code (like other JSON mappers used to do). The class ISO8601Utils.kt has been added only on the unit test, so it doesn't ship with the library core.

[ppamorim]

@damphat How do you suggest to implement the JSON.stringify(date) feature?

[damphat]

@ppamorim

In java-script world:

JSON.stringify([new Date()])

// return "["2017-08-17T15:16:49.170Z"]"

As you see java-script use ISO8601 to stringify a date by default.
So I suggest 2 changes for klaxon:

• fun render() should convert Date to ISO string
• fun JsonObject.date() should have default formatter like this: date(fieldName, formatter = iso8601)

[ppamorim]

@damphat fun render() needs to convert the date using a format provided. This format can be an lambda like the parse used to do. I disagree that fun JsonObject.date() needs to have a default formatter, we need to let it open to any type of date formatting, including this ISO formatter will create a dependency that is not needed.
@cbeust What you think about it?

[damphat]

I understand what you said. But I gave the suggestion base on 3 things:

1. Modern javascript stringify a Date object by converting it to ISOString.
2. We can override the default parameter in kotlin

it.date("birthday")  // default to iso, keep the code less verbose
it.date("birthday", customFormatter) // provide custom formatter if needed

3. Why default formatter for render? Because the code below should worked:

render(Date()) // this API is already use by many projects, to keep this work we must provide default formatter 

PS: I'm not sure about my ideas, I think JsonObject.date() is great for json query, keep moving!

[ppamorim]

@damphat The default formatter involves the Java class Instant?

[ppamorim]

@damphat Now the stringify is working. Check the test class PrettyPrintTest. It uses an class called Modifier to provide the lambda used to format de Date object into a String. Is this the best solution? No. But better than nothing.

[damphat]

You added the concept Modifier(val parseDate: (Date) -> String), so I have some questions:

1. What is the benefit of class Modifier(val parseDate: (Date) -> String) over a simple lamda (Date) -> String
2. why did you name the param parseDate, should it be renderDate or stringifyDate?

[ppamorim]

1: Modulability, it maintains the code more consisce in case of implementation of new modifiers and make the code more reability and less confusing. A single lambda function seems to be enough but adds complexibility.
2: There is no param parseDate, the name is renderDateToString, check it again please.

[damphat]

@ppamorim
class Modifier is wrong way to go.
Not as you expected, Modifier(val renderDateToString: (Date) -> String) WILL NOT ALLOW adding new modifiers in future. If you add more properties to class Modifier, many test-cases and projects that depends on klaxon will fail to compile.

Note that you are working with public apis so that is not acceptable. That idea may still a good choice for application, non-public code (implementation code).

[ppamorim]

How many times you will repeat saying that I am not working with a application? That's very rude and doesn't improve anything on that conversation. I will not change until @cbeust do a review.

[damphat]

Sorry @ppamorim, I think you are active and full energy when you contribute to open-source projects. Don't care much about me and my words. Focus more on your job, I think you can learn something and 'make the world a better place'.

[cbeust]

Sorry for the late review.

It seems to me this pull request is obsoleted by the latest addition to Klaxon: object binding with custom converters.

If I wanted to convert a date, I would simply provide my own converter with the correct expected format before I run the parsing.

Should we close this PR?