cbeust / klaxon

A JSON parser for Kotlin
Apache License 2.0
1.86k stars 121 forks source link

Simple pretty printing??????? #217

Open camdenorrb opened 6 years ago

camdenorrb commented 6 years ago

Add a parameter to toJsonString to take in a boolean for whether or not it should pretty print so we don't have to reread the string into a JsonObject and then pretty print

Example of how it should not be: val builder = StringBuilder(Klaxon().toJsonString(object)) val result = (Parser().parse(builder) as JsonObject).toJsonString(true)

Example of how it should be: val result = Klaxon().toJsonString(object, prettyPrint = true)

Not only will this allow for less overhead, but the syntax would make more sense while also being pretty

cbeust commented 6 years ago

The reason why this is a bit harder than it looks is because in order for this to work, I need to alter the Converter interface so that its toJson(): String method accepts two additional parameters: an indent and a boolean prettyPrint parameter. Otherwise, custom converters will not be able to pretty print objects.

Not a huge deal, but this means it's a breaking change, and therefore will require a major version bump.

camdenorrb commented 6 years ago

@cbeust Not if you overload or use default values for the params

cbeust commented 6 years ago

No, if you add parameters to an interface, implementers of that interface will have to add these parameters too, whether they have a default value or not.

cbeust commented 6 years ago

Just to clarify, I don't have a problem with breaking changes, just wondering if it's worth polluting the Converter interface with pretty printing concerns.

camdenorrb commented 6 years ago

Then overloading? @cbeust

cbeust commented 6 years ago

Overloading? Like I said, it's still a breaking change, even with default values. Try it.

camdenorrb commented 6 years ago

Okayz, hope to see it in a major release then :P

markusmoormann commented 5 years ago

@cbeust what about an additional method? toPrettyJsonString

current Workaround would be an extension function

fun Klaxon.toPrettyJsonString(value: Any): String {
    val builder = StringBuilder(Klaxon().toJsonString(value))
    return (Parser().parse(builder) as JsonBase).toJsonString(true)
}
camdenorrb commented 5 years ago

@markusmoormann This isn't a syntax issue, the whole idea of converting it to a string just to parse it again just to convert it again is just stupid.

markusmoormann commented 5 years ago

@camdenorrb sure this is not ideal. It's just a quick fix to cleanly code against something.. But I'm still wondering whether it would be bad to add a new function. I think it's quite common you want to have preformated json you want to export. How to implement is anther question to be discussed

camdenorrb commented 5 years ago

I would personally like to see just a simple boolean to determine whether to pretty print

raderio commented 5 years ago

I just don't get it val result = Klaxon().toJsonString(object) will work as now with prettyPrint by default false val result = Klaxon().toJsonString(object, prettyPrint = true) will do pretty print Am I missing something?

camdenorrb commented 5 years ago

@raderio You're missing the fact that it doesn't exist

camdenorrb commented 5 years ago

Any updates?

cbeust commented 5 years ago

This is all getting a bit confusing :)

How about people interested in a change send a pull request so I can see what they want?

BinaryShrub commented 5 years ago

I think people (including myself) just want the ability to call:

val result = Klaxon().toJsonString(object, prettyPrint = true)

right now there is no way to prettyPrint, the README.md is also outdate and says you can use toJsonString(true) which doesnt exist?

sghpjuikit commented 5 years ago

@cbeust

I need to alter the Converter interface so that its toJson(): String method accepts two additional parameters: an indent and a boolean prettyPrint parameter.

No, the toJson(): String should simply not return String in the first place, but a JsonBase, which would unify fromString and 'toString'. The JsonBase should be the intermediary between Kotlin object and String, in order to have a stage where the json is represented in its AST form.

Honestly, I do not understand why it hasn't been done this way, especially since the underlying types are even there, they are just used only in one direction. This is case of bad design, which lead to this (and probably many other) problems. https://github.com/spray/spray-json

Is there a way to convert Kotlin object to JsonBase?

cbeust commented 5 years ago

@sghpjuikit As the saying goes, any problem in computer science can be solved by adding one layer of indirection :-)

I agree with your suggestion and I'm open to introducing such a breaking change, I'll just bump the major version.

Would you be interested in sending a pull request? (just making sure we are not going to duplicate work, but it might take me a bit before I get around to implementing your suggestion).

sghpjuikit commented 5 years ago

See #280