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

Added CSV quotation for delimiters #34

Merged
merged 1 commit into from
Feb 10, 2014

Conversation

vratiu
Copy link

@vratiu vratiu commented Feb 10, 2014

I've encountered some issues when trying to convert files that had commas, CRLF or double quotes in filed.
I followed directions in http:https://tools.ietf.org/html/rfc4180 "2. Definition of the CSV Format" and enclosed the value with double quotes if it containes delimiters or double quotes

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 349af44 on vratiu:improved_csv_quote into 0cfb22e on SheetJS:master.

@SheetJSDev
Copy link
Contributor

@vratiu Thanks :)

A few changes (and if you can squash/amend, that would be great):

  1. opts = opts || {}; appears to have the wrong indentation (you added another level?)

  2. Add the line "Fixes #32" at the bottom of the commit message (I think your patch would also resolve added case for german languaged Worksheets sheetjs#32)

@SheetJSDev
Copy link
Contributor

@vratiu I should add the notes to this repo, but please look over https://github.com/SheetJS/js-xlsx/blob/master/CONTRIBUTING.md and make sure everything looks good.

@vratiu
Copy link
Author

vratiu commented Feb 10, 2014

@SheetJSDev I've updated the code. I hope all looks well.
Everything looks good on the contributors side.

@SheetJSDev
Copy link
Contributor

Can you squash the two commits into one? (make it one commit). Then I can merge :)

SheetJSDev added a commit that referenced this pull request Feb 10, 2014
Added CSV quotation for delimiters
@SheetJSDev SheetJSDev merged commit d453e12 into SheetJS:master Feb 10, 2014
@SheetJSDev
Copy link
Contributor

@vratiu Perfect! Thank you very much

@vratiu
Copy link
Author

vratiu commented Feb 10, 2014

No problem. Thanks for creating the library :-)

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.

None yet

3 participants