-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fixed #532 - Implemented onlyQuoteStrings #534
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for your contribution.
The review looks good, I leaved a comment which will improve the current code. It will be great if you can fix it also.
Could you please document the new option in the parse section of our docs. See:
https://github.com/mholt/PapaParse/blob/master/docs/docs.html#L225
papaparse.js
Outdated
@@ -412,9 +420,15 @@ | |||
if (str.constructor === Date) | |||
return JSON.stringify(str).slice(1, 25); | |||
|
|||
// We must check, whether the data type is string already here, | |||
// since all data types are converted into strings in the next line | |||
var onlyQuoteStrings = _onlyQuoteStrings && typeof str === 'string'; |
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.
I think we should rename str into value here to make code clearer.
|
After reading the docs I'm wondering if we should not allow three options on the quotes parameters:
With a second though I'm wondering if we should allow to pass a function to the quotes argument which will accept the value and the col and return true if the field needs quoting. This will prevent adding more settings like: 'only-numbers' and give the full power to the end user. For the string use case it will be:
What do you think? |
I think, adding a function is a quite nice idea! This makes it much more flexible. I would like to keep the true/false to be backward compatible, thus upgrading to a newer version does not break existing applications (e.g., true/false will be internally implemented as a function as well), thus:
If you go for it, I would handle the implementation for this as well. |
Your proposal makes sense for me. Please go for the implementation! |
@boldt Are you still planning on implementing the function option? I can help out if you want. |
Much needed feature for our team if you're looking for any motivation. ;-) |
This is a fix for #532