add InvalidSuriSyntaxError, MoabRuntimeError, MoabStandardError #160
Conversation
| @@ -259,7 +259,7 @@ def create_tarfile(tar_pathname = nil) | |||
| rescue | |||
| shell_execute(tar_cmd.sub('--force-local', '')) | |||
| end | |||
| raise "Unable to create tarfile #{tar_pathname}" unless tar_pathname.exist? | |||
| raise(MoabRuntimeError, "Unable to create tarfile #{tar_pathname}") unless tar_pathname.exist? | |||
There was a problem hiding this comment.
This one is trivial: why the parens? I don't think we need them, and it'd look a tad bit more idiomatic to my eyes without them (like before). Totally a style preference, so this isn't blocking; I'm just curious why you added parens to each of these.
There was a problem hiding this comment.
I believed it was easier to read with parens when followed by unless or if. Perhaps I overdid it.
| @@ -1,13 +1,9 @@ | |||
| module Moab | |||
| class ObjectNotFoundException < RuntimeError | |||
| end | |||
| class MoabRuntimeError < RuntimeError; end | |||
There was a problem hiding this comment.
I wonder whether you're open to declaring a single MoabError class rather than two classes. Or maybe there is some value to distinguishing between a Moab "standard" error and a Moab "runtime" error? That was there is a single class for all custom errors in this codebase.
There was a problem hiding this comment.
The value is that one of the exceptions that used to be thrown is StandardError, so it seemed useful to do it this way. My goal is to do a minor release of MoabVersioning so there is absolutely no ill effect to any app using the gem -- I don't want to cause work for any consumers except pres_cat, where John and Peter did a kludge because they couldn't do this PR at the time. (See sul-dlss/preservation_catalog#1229 (comment), see also https://github.com/sul-dlss/preservation_catalog/pull/1207/files)
There was a problem hiding this comment.
@ndushay Hmm, I'm not sure I follow. Are you saying that the value of making subclasses of StandardError and RuntimeError is that downstream code can rescue the RuntimeError children without accidentally rescuing the StandardError children?
ndushay
force-pushed
the
exception-ize
branch
from
127246d
to
c17df9c
Compare
| class InvalidMetadataException < RuntimeError | ||
| end | ||
|
|
||
| class ValidationException < RuntimeError |
There was a problem hiding this comment.
ValidationException is not used.
Why was this change made?
Closes #159
To fix a kludge in preservation_catalog (see https://github.com/sul-dlss/preservation_catalog/pull/1207/files) that would become more onerous as prescat gets more API functionality to replace sdr-services-app.
Was the usage documentation (e.g. wiki, README) updated?
N/A