Improved error handling #6
Conversation
|
@EreMaijala, I suspect that you do not use this tool, so if you prefer for me to find a different reviewer, just let me know! :-) |
There was a problem hiding this comment.
@demiankatz Tested and works fine. I left one comment about the code style in catching the exceptions. Feel free to ignore if you disagree, but I'd say it would be cleaner to do like this:
try {
} catch (OaiException $e) {
} catch (\Exception $e) {
}
| @@ -345,8 +346,14 @@ protected function execute(InputInterface $input, OutputInterface $output) | |||
| try { | |||
| $this->harvestSingleRepository($input, $output, $target, $settings); | |||
| } catch (\Exception $e) { | |||
| $this->writeLine($e->getMessage()); | |||
| return 1; | |||
| if ($e instanceof OaiException | |||
There was a problem hiding this comment.
I think it would be cleaner to catch OaiException in its own catch block. But one could argue that noRecordsMatch shouldn't even cause an exception since it can happen during normal operation.
There was a problem hiding this comment.
I agree that it's strange to treat noRecordsMatch as an error, but since the OAI-PMH protocol treats it as such, I think it's okay for us to do the same for the sake of consistency.
Regarding a separate catch block for the OaiException, I did consider that, but since our special case relies not just on the exception class, but also on a specific code, I don't think I can do that without having to duplicate code, so this approach seemed a little more concise. If you can think of a more elegant approach, though, I'm open to suggestions!
There was a problem hiding this comment.
I think that's ok. I would have happily duplicated the lines, but I understand the desire to avoid that.
| @@ -345,8 +346,14 @@ protected function execute(InputInterface $input, OutputInterface $output) | |||
| try { | |||
| $this->harvestSingleRepository($input, $output, $target, $settings); | |||
| } catch (\Exception $e) { | |||
| $this->writeLine($e->getMessage()); | |||
| return 1; | |||
| if ($e instanceof OaiException | |||
There was a problem hiding this comment.
I think that's ok. I would have happily duplicated the lines, but I understand the desire to avoid that.
This PR improves error handling in the harvester in a few ways: