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

Generic exceptions should be replaced with more specific exceptions #2

Open
nickma42 opened this issue Oct 16, 2018 · 2 comments
Open
Assignees

Comments

@nickma42
Copy link

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 nickma42 changed the title Ambiguous exceptions should be replaced with more specific exceptions Generic exceptions should be replaced with more specific exceptions Oct 16, 2018
@nickma42
Copy link
Author

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.

@offdev
Copy link
Owner

offdev commented Feb 25, 2019

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 offdev self-assigned this Mar 2, 2019
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

No branches or pull requests

2 participants