Throw exceptions instead of using status codes #150
Conversation
| catch (Exception e) { | ||
| log.error("publish failed: ${e.getMessage()}", pkg.meta) | ||
| } | ||
| publish() | ||
| } else { | ||
| log.info("not publishing: ${pkg} [unsuccessful session]") |
There was a problem hiding this comment.
I think this should throw an exception, too, but I don't know which one
There was a problem hiding this comment.
There was a problem hiding this comment.
All of those are for very specific Java features, so doesn't seem right. They all inherit from IOException, so could just use that one.
But, IOException is for failures that the user has no control over (network failures, etc.) - while here, it looks like the user should've checked that the session is valid. If that's the case, then IllegalStateException might be more appropriate.
| // NOTE: push does NOT update local registry | ||
| expect: | ||
| makeWriteProduct().pkg.install() // try to merge existing metadata |
There was a problem hiding this comment.
This actually fails!
But the existing test doesn't check the status code.
| when: | ||
| makeWriteProduct(skip_meta) // still fails on implicit metadata | ||
| then: | ||
| thrown(com.quiltdata.quiltcore.workflows.WorkflowException) |
There was a problem hiding this comment.
I see you already defined one here.
quiltcore.package.PushException?
There was a problem hiding this comment.
I actually never created our own PushException, but just used java.io.IOException. Still don't know what's best.
There was a problem hiding this comment.
java.io.IOException is fine. The user shouldn't really have control over the Session.
No description provided.