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

Fix/newlines in cells #114

Merged
merged 2 commits into from
May 20, 2016
Merged

Conversation

dbuentello
Copy link
Contributor

Issue: Newlines are not preserved within a cell's content.
Cause: JSON.stringify escapes \n and you end up \\n instead.
Fix: Replace occurrences of \\n with \n in the stringifiedElement (before the row delimiter).

Includes appropriate tests

@coveralls
Copy link

coveralls commented May 20, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling c2b0f81 on dbuentello:fix/newlines-in-cells into dbdc877 on zemirco:master.

@dbuentello
Copy link
Contributor Author

resolves #91

@knownasilya
Copy link
Collaborator

Excellent!

@knownasilya knownasilya merged commit a8c63f4 into zemirco:master May 20, 2016
knownasilya pushed a commit that referenced this pull request May 20, 2016
- Fix new lines, resolves #91, see #114
@dbuentello dbuentello deleted the fix/newlines-in-cells branch May 20, 2016 19:52
@grimor
Copy link

grimor commented Jun 13, 2016

It opens correctly in OpenOffice, but in Excel it translates new line as new row. It breaks all displayed data (can't correctly figure out that newline char is within " " and shouldn't create new row). I reverted to v3.4.1 to solve that. Don't know why it was changed. You can't display multi line text within cell (as far I know)

@knownasilya
Copy link
Collaborator

@grimor http://superuser.com/a/73851/209612 are you sure it's not an issue with your version of Excel?

@grimor
Copy link

grimor commented Jun 13, 2016

Ok, even if it's possible to create new lines, it doesn't get imported correctly for me (Excel 2010). It converts new lines intro new row (as CSV contains each row in new line). It doesn't parse that it's between double quotes/

@knownasilya
Copy link
Collaborator

@dbuentello what version of Excel do you have?

@dbuentello
Copy link
Contributor Author

Excel 2015 for Mac, also tested on Windows with Excel 2016

@dbuentello
Copy link
Contributor Author

dbuentello commented Jun 14, 2016

@grimor can you run code below and upload csv

var json2csv = require('./lib/json2csv');
var fs = require('fs');

var data = [{
  col1: 'This doesnt have newline',
  col2: 'Line1\nLine2',
  col3: 'This doesnt have newline'
},
{
  col1: 'This doesnt have newline',
  col2: 'Line1\nLine2\n\nline 3',
  col3: 'This doesnt have newline'
}]
json2csv({ data: data }, function(err, csv) {
  if (err) {
    console.log(err);
  }
  fs.writeFile('file.csv', csv, function(err) {
    if (err) {
      throw err;
    }
    console.log('file saved');
  });
});

@leachiM2k
Copy link

Hey, I'm using Excel 2010 and have the same issue. The only solution was to downgrade to 3.4.2

@dbuentello
The result of your script is

"col1","col2","col3"
"This doesnt have newline","Line1
Line2","This doesnt have newline"
"This doesnt have newline","Line1
Line2

line 3","This doesnt have newline"

I tried to generate a multi-line CSV in Excel and I've found out, that Excel uses different characters to determine normal (row) line break and a multi-line cell line break.
Line Breaks in Cell are only determined if they are \n only and all other line breaks are \r or \r\n

@knownasilya
Copy link
Collaborator

Good catch! So the split needs to be more specific it seems.

knownasilya pushed a commit that referenced this pull request Jun 29, 2016
This reverts commit a8c63f4.

Conflicts:
	test/helpers/load-fixtures.js
	test/index.js
knownasilya pushed a commit that referenced this pull request Jun 29, 2016
* Revert #114, due to more issues
* Update npmignore
* Add a changelog
* Updatee readme
@knownasilya
Copy link
Collaborator

I've reverted this PR in 3.5.1

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.

5 participants