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

feat: added optional delimiters #8

Merged
merged 3 commits into from
Aug 20, 2018
Merged

feat: added optional delimiters #8

merged 3 commits into from
Aug 20, 2018

Conversation

HKskn
Copy link
Contributor

@HKskn HKskn commented Aug 17, 2018

I think it needs optional delimiters.

@coveralls
Copy link

coveralls commented Aug 17, 2018

Pull Request Test Coverage Report for Build 40

  • 5 of 5 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 37: 0.0%
Covered Lines: 78
Relevant Lines: 78

💛 - Coveralls

@ryu1kn
Copy link
Owner

ryu1kn commented Aug 18, 2018

Hi @HKskn , thank you for taking time to contribute to the project! but I've got a different idea on supporting other delimiters. The current FieldStringifier class is actually specific to CSV and respects RFC4180; so I wouldn't use the same logic if I use other characters for a delimiter.

I've started thinking to support TSV (even this library is named csv-writer) after I've got some requests. Do you have some specific delimiters you want to use? Do you mind sharing your context/use-case?

@HKskn
Copy link
Contributor Author

HKskn commented Aug 18, 2018

Hi @ryu1kn, you are right. In my case, csv files formatted with delimiter ',' can't fit columns correctly on excel. It needs to use delimiter ';'. I think restricting delimiters is not good, there can be another delimiters. Thanks.

@ryu1kn
Copy link
Owner

ryu1kn commented Aug 18, 2018

Do you mean the CSV file generated by csv-writer doesn't look right on Excel? That sounds like the CSV is not compatible with the format Excel is expecting. Do you mind sharing the simple code snipets and sample data that reproduce the CSV along with the expected results (in the form of screenshots/excel file/...) so that I can have a closer look? Also can you let me know the OS and node versions?

It's also interesting that if you use ; in CSV file as a delimiter then Excel can parse it correctly, if we use ; it's not Comma Separated Value anymore...

@ryu1kn
Copy link
Owner

ryu1kn commented Aug 19, 2018

It's also interesting that if you use ; in CSV file as a delimiter then Excel can parse it correctly, if we use ; it's not Comma Separated Value anymore...

Ok, I found that, on Windows, depending on the "Regional and Language Options" settings, Excel assumes different characters for the field delimiter...

Seems we do need to support ; as a delimiter. Now I need to find what is the escaping rule for the field value when ; is used as a separator.

@ryu1kn ryu1kn mentioned this pull request Aug 19, 2018
@ryu1kn
Copy link
Owner

ryu1kn commented Aug 19, 2018

Now I need to find what is the escaping rule for the field value when ; is used as a separator.

As far as i can see from a quick play around with Excel on windows with ; as a list separator, passing a field delimiter to FieldStringifier should do the job. But I want to limit the delimiter to only either comma or semicolon because, for example, TAB character as a delimiter requires different stringifying rules.

So I’m going to merge your PR and continue work on it before i release the next version.

@ryu1kn ryu1kn merged commit cb1352e into ryu1kn:master Aug 20, 2018
@ryu1kn
Copy link
Owner

ryu1kn commented Aug 20, 2018

Merged! I'll let you know once I release the new version 👍

@ryu1kn
Copy link
Owner

ryu1kn commented Aug 20, 2018

Released as v1.1.0. Thank you again for your contribution!!

@ryu1kn ryu1kn mentioned this pull request Nov 26, 2018
@willvedd willvedd mentioned this pull request May 14, 2021
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