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

Parameter unwindPath for multiple fields (#174) #183

Merged
merged 2 commits into from
Jul 10, 2017

Conversation

eduardomourar
Copy link
Contributor

This relates to #174. It adds functionalities as described below:

  • unwindPath parameters changed to array of strings (on both library and CLI);
  • Add logic to unwind multiple times;
  • Add unit test.

@coveralls
Copy link

coveralls commented Jul 10, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 976d04f on eduardomourar:master into 62e8b0c on zemirco:master.

Copy link
Collaborator

@knownasilya knownasilya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple tweaks, but otherwise looks great.
Of course, I'll have to release this as v4, since it's a breaking change.

README.md Outdated
var fields = ['carModel', 'price', 'items.name', 'items.color', 'items.items.position', 'items.items.color'];
var myCars = [
{
"carModel": "BMW",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make the spacing 2 spaces instead of the 4 or tabs that it currently is. Just so it's consistent with the other examples. Thanks!

lib/json2csv.js Outdated
@@ -99,7 +99,7 @@ function checkParams(params) {
return Object.keys(item);
});

dataFields = lodashFlatten(dataFields);
dataFields = lodashFlatten(dataFields);//_.flattenDeep(params.data);//
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the flattenDeep be used or removed?

lib/json2csv.js Outdated
if (params.unwindPath) {
function createDataRows(originalData, unwindPaths) {
var dataRows = originalData;
if (unwindPaths.length) {
dataRows = [];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like dataRows gets overwritten right away, why not set it to an empty array on line 297, and just return originalData if unwindPaths.length is falsy.

params.unwindPath = params.unwindPath || [];

// if unwindPath is not in array [{}], then just create 1 item array.
if (!Array.isArray(params.unwindPath)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this part of code, make the change compatible with old single string parameter. I have not extensively tested, but it should work fine. So I would say no breaking changes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, excellent!

@coveralls
Copy link

coveralls commented Jul 10, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 2c1b159 on eduardomourar:master into 62e8b0c on zemirco:master.

lib/json2csv.js Outdated
@@ -307,6 +305,8 @@ function createDataRows(originalData, unwindPaths) {

Array.prototype.push.apply(dataRows, dataRow);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? Should be able to do dataRows.push(dataRow)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually necessary. Check out this link, but basically we need to merge two arrays (dataRows and dataRow).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed that it was an apply and not a call.

@knownasilya knownasilya merged commit fbcaa10 into zemirco:master Jul 10, 2017
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.

3 participants