phase2 / rig

Outrigger command line tool
MIT License
11 stars 8 forks source link

Success/error refactoring #84

Closed febbraro closed 6 years ago

febbraro commented 6 years ago

This allows us to centrally control how things get handled for success and error. It also means we don't need to config which commands provide notifications or not b/c we can just not call those or we can make cmd.Success and cmd.NotifySuccess and cmd.Error and cmd.NotifyError and that leaves it up to each command to control if it integrates with notifications or not?

grayside commented 6 years ago

I like a lot about this. If a given command wants a non-default handling of the notification they can replace the relevant function.

The downside is we will need to have the Run callback for each command do something like:

if err == nil {
  return cmd.Success(...)
} else {
  return cmd.Error(...)
}

The other thing is that we should have the command name in the message, and I'd prefer not to have that explicitly set by the command, it should just be part of the command calling the notification. Can BaseCommand access the calling command name? Maybe it needs to be passed the context as well, cmd.Success(ctx, message) and cmd.Error(ctx, message, code)

febbraro commented 6 years ago

I'm not sure I follow the comment about the Run callback, or really how it is different than what we do now which is cmd.out.Error.Fatal when something really bad happens, or return nil if we end in a good state.

grayside commented 6 years ago

In this case, it's a very minor point that if we switch to a model where we return an error or a nil to indicate success/failure, we will first need to evaluate that err state to take one or other action.

An alternative might be:

return cmd.Complete(ctx, err, messageOnError, codeOnError)

Then the evaluation of whether this was a success or failure can be done centrally. However, I think the finer-grained control of your existing approach is better.

febbraro commented 6 years ago

Did another pass, this needs to be tested, I only verified that it compiles

febbraro commented 6 years ago

So my rebasing has now pulled everything we have put into develop into this PR even though it is already in the target branch. Apparently I have no idea how to rebase and manage git branches. sigh