search

ralfstx / minimal-json

A fast and small JSON parser and writer for Java
MIT License
732 stars 185 forks source link

Added Json.value(object) method #80

Closed pbi-qfs closed 7 years ago

pbi-qfs commented 7 years ago

To create JsonValues directly from complex java objects.

bernardosulzbach commented 7 years ago

Style is really inconsistent, mostly regarding whitespace. There are some unnecessary parentheses too.

Was this ever discussed? Would this be useful in practice?

Additionally, a "object to JSON mapper" would - as I see it - use reflection to create the object as well as possible instead of just calling toString(). Usually in practice you only need parts of an object and can manually write how to encode / decode (this is also important) your objects.

Lastly, if I would need this - for instance - to log some custom objects as JSON and could allow toString(), calling it in the code and using the returned value seems more explicit than calling value(Object) as the reviewer would have to look the documentation and see what value(Object) does with that type of object.

pbi-qfs commented 7 years ago

@ mafagafogigante

Thanks for your comments, I adapted the whitespace to the style of this project - anything I missed?

Even if minimal-json explicitly does not want to provide a full-fledged object-json mapping (as e.g. Gson does), the ability to convert collections (of collections of ...) seems to be a missing piece of the existing Json.value(...) method, since it does not support any recursion out-of-the-box, yet.

In other discussions, it was made clear that reflection is not the way to go for minimal-json. I second you in that for some objects you want to explicitly specify how they are serialized to a JsonValue. The cleanest way to do this (and to still support recursive serialization) is an interface implementation, as I have sketched out in e4e66e3e7b. Would this be the route to follow?

bernardosulzbach commented 7 years ago

Thanks for your comments, I adapted the whitespace to the style of this project - anything I missed?

I don't think so.

(...) the ability to convert collections (of collections of ...) seems to be a missing piece of the existing Json.value(...) method, since it does not support any recursion out-of-the-box, yet.

I agree that one might expect this to work, but I don't see it as a feature that fits this project.

In other discussions, it was made clear that reflection is not the way to go for minimal-json.

I was just mentioning that if we were going to support object mapping, it would be required.

Would this be the route to follow?

If we are going to support recursive conversion, then yes. I'm not the maintainer, just someone that makes use of and likes this project, so I don't have a final word and maybe people will agree with you - that we need to be able to convert collections (of collections...) of objects out of the box. If that is the case, then I think being able to define how your objects would be converted so that minimal-json can convert collections (of collections...) of these objects is the right thing to do.

pbi-qfs commented 7 years ago

If that is the case, then I think being able to define how your objects would be converted so that minimal-json can convert collections (of collections...) of these objects is the right thing to do.

Since the PR #59 also contained this functionality (in parts), and it was the only feature which was missing to replace a heavy-weight Json library in our (big) project with json-minimal, I would argue that there is some need for the extension of Json.value().

I closed this PR in favor of PR #81 containing the interface to control object serialization.