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

Strict record handling #53

Merged
merged 8 commits into from
Mar 3, 2015
Merged

Strict record handling #53

merged 8 commits into from
Mar 3, 2015

Conversation

derjust
Copy link
Contributor

@derjust derjust commented Aug 6, 2014

Hi!
I have the requirement to fail if the records are incomplete. Up to now, fast-csv is quite polite and just consideres an incomplete line as the first field/property and continues.

A workaround wouuld be to check in the validate function if the row is incomplete. but based on the amount of fields, it is very complex to identify a record which may have some fields left empty (which might be valid) and some literally missing.

Therefore introduced a new property (on default, the existing behavior remains unchanged) to emit data-invalid events on incomplete rows.

@doug-martin
Copy link
Contributor

Thanks for the pull request! Can you please add some tests

@derjust derjust mentioned this pull request Aug 20, 2014
Conflicts:
	lib/parser_stream.js
	test/fast-csv.test.js
… cases where there are too few columns provided
@derjust
Copy link
Contributor Author

derjust commented Mar 3, 2015

Added testcase to describe the intended behavior and updated to the latest HEAD

assert.deepEqual(actual, expected25_invalid);
reachedInvalid = true;
})
.on("error", function(e){console.log(e)})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should error on here you can just do on("error", next)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with e859b69
Thanks

@doug-martin
Copy link
Contributor

Other than my one comment this looks good Ill merge once fixed.

Thanks again
-Doug

doug-martin added a commit that referenced this pull request Mar 3, 2015
@doug-martin doug-martin merged commit 4558ccc into C2FO:master Mar 3, 2015
doug-martin added a commit to doug-martin/fast-csv that referenced this pull request Mar 3, 2015
* Strict record handling C2FO#53 - @derjust
@doug-martin doug-martin mentioned this pull request Mar 3, 2015
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.

2 participants