csv icon indicating copy to clipboard operation
csv copied to clipboard

Generic exceptions should be replaced with more specific exceptions

Open nickma42 opened this issue 6 years ago • 2 comments

Really tidy little CSV parser, my only issue is if I'm wanting to catch errors and return my own messaging to users, I'd currently have to catch the RuntimeException and then check the message to determine what kind of failure it was.

I think it might be nicer to throw things like InvalidRecordException or StreamNotSeekableException which all extend a common ParserException which can then extend RuntimeException for backwards compatibility.

Happy to do a PR if you think it's a good idea, and thanks for publishing this!

nickma42 avatar Oct 16 '18 16:10 nickma42

And another thought about being able to access the actual validation errors if a record fails validation; at the moment you just check passes() on the Laravel validator here:

https://github.com/offdev/csv/blob/master/src/Parser.php#L281

Maybe the Validator class could mirror the behavior of the validate() method of the underlying Laravel validator instead, which throws a ValidationException with access to the validator so you can pull out the validation messages?

It could look something like this:

try {
    $this->validator->validate($record);
    $this->parseSuccess($record);
} catch (ValidationException $exception) {
    $record->setIsValid(false);
    $this->parseFailed($record, $exception);
}

and parseFailed can then re-throw the exception if throws is enabled. Anything upstream catching those ValidationException exceptions can just do $exception->getValidator() or similar to get the Validator and access the error messages.

nickma42 avatar Oct 16 '18 16:10 nickma42

Sorry for the delayed answer here, but I completely didn't see the open issues...

B2T: I think you made some valid points there. If you want, you can do a PR. If you don't want to, just let me know, so I can do it myself.

offdev avatar Feb 25 '19 17:02 offdev