DiegoZoracKy / convert-excel-to-json

Convert Excel to JSON, mapping sheet columns to object keys.
MIT License
290 stars 91 forks source link

Include Empty Rows #11

Closed harshitgupta closed 6 years ago

harshitgupta commented 6 years ago

Hi,

First of all thank you for the module. Its one of the rare modules that

One problem I found is that it ignores empty lines by default. My app's business logic requires me to pick up rows based upon user-given index values (which include empty lines).

It would be great if you could make it config based rather the current approach.

I looked into the code and found I need to comment following lines in order enable empty lines:

           // if (columnData === '') {
           //     continue;
           // }
        // Cleaning empty
        // rows = rows.filter(v => v !== null && v !== undefined);

Can you please confirm that I am not missing any other lines?

Thanks.

DiegoZoracKy commented 6 years ago

You are right. It does that by default. What do you think about a config to indicate when it should return the empty values (being the current behavior of not returning the empty the default value)?

As you are already on the right snippet of the code, would you like to implement it?

harshitgupta commented 6 years ago

Thank you for the confirmation. I will create the pull request soon.

DiegoZoracKy commented 6 years ago

Hi @harshitgupta,

Are you still interested in working on that? No pressure, just checking to see if we should keep this issue open.

NunoChumbo commented 6 years ago

Hi, i too need for the column value even if cell is empty, but as for the empty lines it may stay as it is. might consider this line if (cell == '!ref' || sheet[cell].v === undefined )

putting the right part || into configuration ([sheet[cell].v === undefined]), something like importnullvalues:true || false Thank you

harshitgupta commented 6 years ago

sorry for the delay but I just created the pull request.

Please let me know if you have any questions.

Thanks @NunoChumbo for the reminder.

DiegoZoracKy commented 6 years ago

Hi @ harshitgupta,

Thanks for creating the PR. It will handle very well this case. I just wrote one comment there that I would like to see your answer before merging it.

harshitgupta commented 6 years ago

@DiegoZoracKy Sure but I don't see any comment there.

screen shot 2018-11-04 at 10 28 14 pm

Could you please point me to it?

DiegoZoracKy commented 6 years ago

Sure, it is on the changes section:

https://github.com/DiegoZoracKy/convert-excel-to-json/pull/20/files

harshitgupta commented 6 years ago

well, it's embarrassing. I still don't see it, neither did i get any notification for it. do you have to assign it to me or something?

DiegoZoracKy commented 6 years ago

That's weird. Anyway, here is the screenshot of it:

img_20181105_025235

harshitgupta commented 6 years ago

On my side its looks like this:

screen shot 2018-11-04 at 10 56 43 pm

no sign of any comment or anything.

But anyways to answer your question I would say that since we are dealing with array which starts with index of 0 and the row at line # 49 always start from 1 (assuming first line is not empty) as first cell will always be A1 in the same case. That means nothing ever will get assign to row[0], thus it will always be undefined.

Second it has never been a problem so far because in the existing using the following code you have been getting rid of rows that are null or undefined.

rows = rows.filter(v => v !== null && v !== undefined);

Please let me know if it makes sense.

DiegoZoracKy commented 6 years ago

Thanks for your answer and your PR @harshitgupta. It is already merged and published on npm (v 1.5.0)

@NunoChumbo might want to take a look.

DiegoZoracKy commented 6 years ago

PR merged: https://github.com/DiegoZoracKy/convert-excel-to-json/pull/20