-
Notifications
You must be signed in to change notification settings - Fork 42
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
Bugfix/complex json array unexpected behavior #81
base: master
Are you sure you want to change the base?
Bugfix/complex json array unexpected behavior #81
Conversation
…sparse so that fillGaps finds the columns for the first row accurately. Updated test to use os.EOL instead of \n for object.js
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) |
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. |
Update iuabhmalat/jsonexport fork with changes from kaue/jsonexport
Hi @kaue , @AckerApple , Abhi |
@kaue @AckerApple - any chance we can get this merged? |
Great callouts. I’ll aim to review within 4 days |
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 |
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 |
@@ -121,9 +120,8 @@ describe('Object', () => { | |||
} | |||
], {}) | |||
|
|||
assert.equal(csv, `a,b,c,d,e\n0,,,1,this`) | |||
assert.equal(csv, `a,b,c,d,e`+ os.EOL +`0,,,1,this`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, no real changes related to this pull request. Looking for supporting unit tests and none found
@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 |
@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. |
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:
I was expecting there to be two trailing commas on line 2 but there was not 😢 Expected result:
|
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:
Todos
Steps to Test or Reproduce
See issue #78 to reproduce the bug in 3.0.1
Impacted Areas in Application
List general components of the application that this PR will affect: