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

Fixed #532 - Implemented onlyQuoteStrings #534

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

boldt
Copy link

@boldt boldt commented Jul 6, 2018

This is a fix for #532

Copy link
Collaborator

@pokoli pokoli left a 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';
Copy link
Collaborator

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.

@boldt
Copy link
Author

boldt commented Jul 6, 2018

  • I renamed str into value
  • I added a documentation for the new property
  • I also formatted the unparse config as an unordered list

@pokoli
Copy link
Collaborator

pokoli commented Jul 9, 2018

After reading the docs I'm wondering if we should not allow three options on the quotes parameters:

  • false
  • true
  • 'only-strings'

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:

config: {
'quotes': function(value, col) { return typeof value == 'string' },
}

What do you think?

@boldt
Copy link
Author

boldt commented Jul 16, 2018

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:

  • false - do not add quotes (default)
'quotes': function(value, col) { return false; }
  • true - add quotes
'quotes': function(value, col) { return false; }
  • a function is provided - use the function (like your example)

If you go for it, I would handle the implementation for this as well.

@pokoli
Copy link
Collaborator

pokoli commented Jul 17, 2018

Your proposal makes sense for me. Please go for the implementation!

@MonkeyDZeke
Copy link
Contributor

@boldt Are you still planning on implementing the function option? I can help out if you want.

@closet6
Copy link

closet6 commented Dec 24, 2018

@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. ;-)

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

4 participants