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

Hooks relying on a return value of false to skip entries makes parsing values that resolve to false impossible #44

Closed
evanplaice opened this issue Sep 9, 2015 · 1 comment
Labels
Milestone

Comments

@evanplaice
Copy link
Owner

From whitesid...@gmail.com on August 11, 2014 07:06:16

The onParseEntry callback function provides plenty of scope to parse entries to numerical values, Booleans, convert special cases to other values, etc, but since it always skips a value when a hook to boolean false, you cannot use it to handle values that parse to boolean false.

Repro example:

var ParseBool = function (value)
{
if (value == "true")
{
return true;
}
else if (value == "false")
{
return false;
}
else
{
return value;
}
}

var contentCSV = "index,isAwesome\n0,true\n,1,false";
var data = $csv.toObjects (contentCSV, {onParseValue : ParseBool});

This will produce a JSON array with two objects of structure { index : someIntString, isAwesome : someBool }, one which looks like {index : "0", isAwesome : true } and one that looks like {index : "1", isAwesome : 0 }.

It would be better is the library used undefined to decide when to skip a value, or simply didn't have a skip-value check when parsing values, as it makes less sense to skip parsing specific values than it does to skip entire entries, for example. I've commented out the if hook !=== false line at 474 of the source (in the endOfValue function of parseEntry) and it gives no detrimental effects.

Original issue: http://code.google.com/p/jquery-csv/issues/detail?id=43

@evanplaice
Copy link
Owner Author

Good point...

Changing the skip indicator value to null instead of false.

This will break the current API so it should be included in the next major version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant