Enable custom return status codes

nickelization commented 9 years ago

This PR allows callbacks to specify custom status codes to be returned as a result of a Clique command being run.

These changes also allow existing callbacks to remain the same - triggering them will cause clique:run to just return 'ok' as before. Callbacks can now also specify custom exit status by returning {exit_status, Status, Result} instead of just Result, which will cause clique:run to return {error, ExitStatus} instead. Finally, internal Clique errors will (for now) default to returning an error code of 1.

This is technically a slight API change, insofar as that it is now possible for clique:run to return {error, StatusCode} as well as 'ok'. However, none of our existing code has a problem with this, so this would only potentially affect third-party code outside of Basho. For anyone who doesn't care about return codes, it's a very simple change to work around, and for someone who does care about return codes, this should be a welcome enhancement.

macintux commented 9 years ago

@nickelization Could you please edit the description rather than appending the edit in the middle? It's a bit confusing, so while I think I understand the full impact of the change + change to the change, it'd be useful to see it laid out as if you had made no changes along the way.

macintux commented 9 years ago

I'd really like to see a test involving a callback that returns an exit status before giving this a thumbs up.

nickelization commented 9 years ago

@macintux Thanks for the review comments! Replies below:

I edited the description to remove/reintegrate the "EDIT" comments into the main description. Thought it might be clearer to explicitly show the edit to the description given that the original commits are still present, and I added some changes after originally submitting the PR, but I can definitely see how it was a bit annoying/awkward to read it the way it was.
Nice catch on the type spec. I fixed something else in the arguments, but didn't touch the return value at all. I'll add your fix too.
Good call on adding a unit test. Clique really needs a lot more tests in general, and I'm worried I might end up having to add a bunch of additional infrastructure to write a test for this one specific thing...but I suppose that's work that has to get done eventually anyway. And, hopefully my gut feeling is wrong and it'll be easy :). I'll take a look into it this afternoon.

nickelization commented 9 years ago

Okay @macintux, review changes are in :). Anything else?

nickelization commented 9 years ago

@borshop merge

basho / clique

Enable custom return status codes #66