Added flattenSeparator option

[fahrenq]

Hi @zemirco,

First of all - thank you very much for this great library.

I've added flattenSeparator option. Basically what it does is allowing us to use flatten: true not only to produce header values like Key1.Key2 but Key1__Key2 also.

Here few things I had to change while implementing this feature:

• In lib/JSON2CSVBase.js we had JSON2CSVBase#flatten(dataRow) method that was implicitly called while processing rows. The bad thing about it - we couldn't access JSON2CSVBase instance (this) from the #flatten() method. I had to return another function from it, that actually handles the logic. As an alternative, we can move the flatten logic outside the class at all. What do you think?

• In README.md I added documentation and added some dots to the end of options definitions (48-69 lines). Sorry that it shows everything as changed, I had to indent all lines to fit newly created option.

Thank you very much,
Looking forward to your comments and approval if everything is ok.

[coveralls]

Coverage remained the same at 100.0% when pulling 73f050c on fahrenq:master into b2690fe on zemirco:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 73f050c on fahrenq:master into b2690fe on zemirco:master.

[knownasilya]

Thanks for the contribution @fahrenq!

[knownasilya]

Released as v4.2.0

[fahrenq]

Thank you very much!

…

On Wed, 1 Aug 2018, 02:36 Ilya Radchenko, ***@***.***> wrote: Released as v4.2.0 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#314 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKLK7Ovbw2x2xrDaHTcLrBWKg0keR7Qkks5uMLGwgaJpZM4VoomQ> .

[juanjoDiaz]

A bit too late but I really don't like this change.

It introduces an unnecessary closure for no reason.
It adds a new option for something that could be achieved simply using the existing fields option.
And most importantly, doesn't test the new feature at all

:/

[juanjoDiaz]

This was adding a very unnecessary closure.

You could have accessed the right this but simple replacing this.flatten by this.flatten.bind(this) or (row => this.flatten(row))

[knownasilya]

True, but that function is called only once, so it's really marginal. Feel free to submit a fix if it's gnawing at you 😉

[knownasilya]

I would probably keep the function, and have it take an object of options that are specific for flatten.

[fahrenq]

@juanjoDiaz

It adds a new option for something that could be achieved simply using the existing fields option.

How? I don't see any way to achieve this with fields option unless I have to dynamically compose fields option, which is super tedious and super unintuitive for those who'll read code later.

And most importantly, doesn't test the new feature at all.

Why? I've added a unit test for this. https://github.com/zemirco/json2csv/pull/314/files#diff-6ddc949419d029ce89b8b8cc1b161136

It introduces an unnecessary closure for no reason.

Sorry, I think you're wrong.

When I'm reading the code of any class, I expect to have access to this in all instance functions no matter how they called elsewhere. While using Function#bind will work as a solution, I see it as a dirty hack in our case, since there's only one right this that we can bind to the function. So why would we do this and make life harder for those who'll want to reuse #flatten?

Thanks.

P.S. (row => this.flatten(row)) this looks like a good solution, tell me if you'd like me to implement it.

[juanjoDiaz]

Just having the array of fields, yeah. Might be a bit tedious if you have too many fields... I can agree with that. In any case, the change is in now. No point crying over spilled milk :) I just want to avoid what happened in version 3.X that we ended up with multiple duplicated and overlapping flags just because of people PRing their solutions for their particular problem.

My bad. Didn't see that test. Then it's only missing the equivalent test for the sync API and the CLI.

Unless using arrow functions, this is bound at call time. Wouldn't you wrap every single function of all the classes in anonymous functions just to always force the call?
What if I want to use flatten somewhere else or extend the code? I'd need to do this.flatten()(row) which doesn't make any sense...

As said, no biggie! The change is in and contribution are always more than welcome. Just trying to get the library in the best possible shape by having open discussions :)

[fahrenq]

Yeah, 100% agree with you. In fact, this function now not flattens a row but returns a function that flattens a row. The name is misleading. Definitely not perfect.

Will PR in future to improve this and do everything as semantically perfect as possible.

Thanks.