-
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
Fix CSV Auto Parse #1609
Fix CSV Auto Parse #1609
Conversation
- Use new `cast` method - Add Custom method to Auto Parse values - Use casting context to avoid parsing values surrounded with quotes
…to feature/csv-parse
* @returns {String|Number|Date} | ||
*/ | ||
csvAutoParse = function (value, context) { | ||
if (context.quoting) { |
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.
Make the if blocks consistent by returning the processed value directly, this will also let you remove else
all over.
* @param {String} value - CSV field value | ||
* @param {Object} context - Context of field value | ||
* @param {Boolean} context.quoting - A boolean indicating if the field was surrounded by quotes. | ||
* @returns {String|Number|Date} |
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.
Boolean
as well?
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.
No
* @param {String} value - The string to test for. | ||
* @returns {Boolean} | ||
*/ | ||
isInt: function (value) { |
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.
Were these taken from csv-parse? If so, add a comment linking to the original code in this, as well as the function below.
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.
Yes, added the reference in the below function. Will add a proper description.
@@ -16,9 +16,11 @@ | |||
"type": "text/javascript", | |||
"exec": [ | |||
"var res = JSON.parse(responseBody);", | |||
"tests['Stringified number was parsed correctly'] = data.num === 1;", |
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.
this test was removed @codenirvana ?
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.
@shamasis It was replaced with an equivalent, as can be seen in the CSV fixture below.
Fixes #1100 #1215 #1346
csv-parse
casting context to avoid parsing values surrounded by quotes