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

Add serialization of object members for csv. #97

Open
wants to merge 2 commits into
base: 2.9
Choose a base branch
from

Conversation

ptahchiev
Copy link

  • Keep track of object nesting, similar to jackson-dataformat-properties

 - Keep track of object nesting, similar to jackson-dataformat-properties
@nstdio
Copy link

nstdio commented Nov 9, 2019

Is there any problem with this PR?

@cowtowncoder
Copy link
Member

@nstdio At this point there is one practical problem (aside from merge conflict): being new feature, this needs to be rebased against 2.11. But I do not remember any specific problem against the concept: my main question would be that since @JsonUnwrapped allows similar usage, whether overlap is problematic. Maybe it isn't, but functionality-wise this covers similar case -- however, I can see how with multi-format output, not using that annotation would be preferable.

I will add this PR to my todo list to have another look as it seems valuable, in helping deal with nested values in CSV.

@nstdio
Copy link

nstdio commented Nov 10, 2019

@cowtowncoder yes, exactly. Also, users not always control/own models and adding a @JsonUnwrappd annotation might be not an option.

@cowtowncoder
Copy link
Member

cowtowncoder commented Nov 26, 2019

Ok. So, finally getting to this one.... and realizing there are actually couple of problems to resolve.

A practical one is that unfortunately patch has re-styled code, making diff unnecessarily large, and adding work when I have to undo many (not all, just many) changes.
Since I will need to rebase it to be from 2.11, manually, these are bit unfortunate.

But a bigger one is this: I don't want additional processing overhead for common case of no nesting. So will need to figure out how to avoid automatic prepending and only change name in cases where it is needed for nesting.

So this may take a bit longer yet.

cowtowncoder added a commit that referenced this pull request Nov 26, 2019
cowtowncoder added a commit that referenced this pull request Nov 26, 2019
@cowtowncoder
Copy link
Member

Another thing I am not sure about is addition of path separator per column; not sure how that would work compared to per document (one for whole CsvSchema) would work. Reformatting is especially disruptive for CsvSchema in general.

@branaway
Copy link

I'd love to see this feature goes in the code. since CVS is much more compact than JSON in dealing with large data set. In my case I have to convert between json and CSV without any predefined POJO involved. This is much needed and I don't want to create a fork out of the official Jackson

@cowtowncoder
Copy link
Member

Note: would like to consider this, but would need rework to merge against 2.13 (next applicable minor version).

@cowtowncoder
Copy link
Member

Ok so due to unfortunate reformatting of this patch (probably accidentally done by some reformatter?), it's bit difficult to follow, but now realized that this would only support serialization, not deserialization.
Not quite sure if that makes sense on its own; users might be surprised to find they can serialize things but not deserialize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants