Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Json.value(object) method #80

Closed
wants to merge 3 commits into from

Conversation

pbi-qfs
Copy link

@pbi-qfs pbi-qfs commented May 30, 2017

To create JsonValues directly from complex java objects.

@bernardosulzbach
Copy link
Contributor

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
Copy link
Author

pbi-qfs commented Jun 1, 2017

@ 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 e4e66e3. Would this be the route to follow?

@bernardosulzbach
Copy link
Contributor

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
Copy link
Author

pbi-qfs commented Jun 1, 2017

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.

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