Bugfix/complex json array unexpected behavior

[iuabhmalat]

Status

Ready. Fixes #78

Description

The first record rows were not properly filled because the rows that were being constructed were sparse. Hence, fillGaps was not able to find the indexes and ignored to fill. My change fills the rows from length of row to current index with empty strings.

Related PRs

List related PRs against other branches:

branch	PR
other_pr_production	link
other_pr_master	link

Todos

• [ ] Tests
• [ x] Documentation

Steps to Test or Reproduce

See issue #78 to reproduce the bug in 3.0.1

git pull --prune
git checkout <feature_branch>
bundle; script/server

Impacted Areas in Application

List general components of the application that this PR will affect:

[AckerApple]

Issue #87 I think is in reference to this. If so lets call it out, resolve merge conflicts, and perhaps include a test if one is not included in this PR.

(Update: I see a test edit and that maybe enough but I cannot fully tell)

[kaue]

indeed @AckerApple, i might be able to help merging this PR this weekend, but if you have time, it would be great to get this merged.

[iuabhmalat]

Hi @kaue , @AckerApple , I have updated my fork with the jsonexport/master branch.This PR is ready for merge. Thank you for your time to review it.

Abhi

[hdwatts]

@kaue @AckerApple - any chance we can get this merged?

[AckerApple]

Great callouts. I’ll aim to review within 4 days

[AckerApple]

I promised to review this but made a mistake and reviewed another PR #90 thinking it was this one

A changelog change was just made and it drew my attention to my mistake.

I hope to review this PR within 4 days of now. I apologize

[AckerApple]

A note that package.json.version will need to be updated. Based on initial review I am recommending that if this code is accepted it be as v3.3.0

A note that the changelog.md should be updated about the changes in this pull request

[AckerApple]

@kaue, I released an update to the online demo only to add UI support for the option "fillTopRow". This option was NOT previously in the demo

Using the updated demo page I could mostly prove that there is in-fact an issue. However, I have never used fillTopRow and do not fully understand the expectations of it.

Using the link pasted below visit the demo, reveal the options panel and toggle on/off fillTopRow option. Analyze the results and they are seemingly incorrect but again I do NOT know for sure

IMPORTANT link reflecting the issue of this ticket

TO CLOSE, I cannot blindly accept this ticket. No supporting unit test. Complexity to debug is too much for unpaid donated time. My time is exhausted. It is someone else's turn to make use of my provided research and feedback to take this pull request further. Otherwise until this issue causes my life issues, IM OUT

[kaue]

@AckerApple I will try to take a look at this issue since I'm more familiar with the fillTopRow implementation.

I may get some time next year to put some effort into refactoring some parts of jsonexport.

I will keep this PR open as WIP for now until i have time to review all of this.

[bestekov]

I tried forking this PR and running it locally to see if it resolved the issue in issue #78 but it did not?

This was my test code:

const csv = require('csv');
const jsonexport = require('jsonexport');

const testArray = [
    { foo: 'a1', bar: 'b1' },
    { foo: 'a2', bar: 'b2', qux: 'd2' },
    { foo: 'a3', bar: 'b3', baz: 'c3' }
  ]

function ExportCSV( array, objName ){
    const opts = {
        textDelimiter : '"',
        fillTopRow : true
    }

    jsonexport ( array, opts , ( err, csv ) => {
        if ( err ) return console.error( err );
        console.log(csv);
    });
}

ExportCSV( testArray, 'test' );

Output:

foo,bar,qux,baz
a1,b1
a2,b2,d2
a3,b3,,c3

I was expecting there to be two trailing commas on line 2 but there was not 😢

Expected result:

foo,bar,qux,baz
a1,b1,,
a2,b2,d2
a3,b3,,c3