From 959cf9de8a980537fd3a82e52dc54d3182d06a84 Mon Sep 17 00:00:00 2001
From: Robert Liebowitz <rliebz@gmail.com>
Date: Tue, 24 Mar 2020 20:32:39 -0400
Subject: [PATCH] Update docs and tests around cli.Exit

Some of the docs were lacking or incomplete concerning how to properly
use cli.Exit, cli.HandleExitCoder, and the ExitErrHandler field on cli.App.
This change aims to clarify the usage of those pieces.

I also noticed that we have two identical functions now: cli.Exit and
cli.NewExitError. Since the latter seems to be more recent and less well
documented, I went ahead and marked it as deprecated so that we can keep
our public interface small.

Also added a missing test case for behavior that's been around a while
but was not documented or tested.

Related: #1089
---
 app.go         |  5 +++--
 errors.go      | 28 +++++++++++++++++++---------
 errors_test.go | 20 ++++++++++++++++++++
 3 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/app.go b/app.go
index d0c8f84..eb68feb 100644
--- a/app.go
+++ b/app.go
@@ -76,8 +76,9 @@ type App struct {
 	Writer io.Writer
 	// ErrWriter writes error output
 	ErrWriter io.Writer
-	// Execute this function to handle ExitErrors. If not provided, HandleExitCoder is provided to
-	// function as a default, so this is optional.
+	// ExitErrHandler processes any error encountered while running an App before
+	// it is returned to the caller. If no function is provided, HandleExitCoder
+	// is used as the default behavior.
 	ExitErrHandler ExitErrHandlerFunc
 	// Other custom info
 	Metadata map[string]interface{}
diff --git a/errors.go b/errors.go
index 344b436..751ef9b 100644
--- a/errors.go
+++ b/errors.go
@@ -17,11 +17,10 @@ var ErrWriter io.Writer = os.Stderr
 // MultiError is an error that wraps multiple errors.
 type MultiError interface {
 	error
-	// Errors returns a copy of the errors slice
 	Errors() []error
 }
 
-// NewMultiError creates a new MultiError. Pass in one or more errors.
+// newMultiError creates a new MultiError. Pass in one or more errors.
 func newMultiError(err ...error) MultiError {
 	ret := multiError(err)
 	return &ret
@@ -65,13 +64,20 @@ type exitError struct {
 	message  interface{}
 }
 
-// NewExitError makes a new *exitError
+// NewExitError calls Exit to create a new ExitCoder.
+//
+// Deprecated: This function is a duplicate of Exit and will eventually be removed.
 func NewExitError(message interface{}, exitCode int) ExitCoder {
 	return Exit(message, exitCode)
 }
 
-// Exit wraps a message and exit code into an ExitCoder suitable for handling by
-// HandleExitCoder
+// Exit wraps a message and exit code into an error, which by default is
+// handled with a call to os.Exit during default error handling.
+//
+// This is the simplest way to trigger a non-zero exit code for an App without
+// having to call os.Exit manually. During testing, this behavior can be avoided
+// by overiding the ExitErrHandler function on an App or the package-global
+// OsExiter function.
 func Exit(message interface{}, exitCode int) ExitCoder {
 	return &exitError{
 		message:  message,
@@ -87,10 +93,14 @@ func (ee *exitError) ExitCode() int {
 	return ee.exitCode
 }
 
-// HandleExitCoder checks if the error fulfills the ExitCoder interface, and if
-// so prints the error to stderr (if it is non-empty) and calls OsExiter with the
-// given exit code.  If the given error is a MultiError, then this func is
-// called on all members of the Errors slice and calls OsExiter with the last exit code.
+// HandleExitCoder handles errors implementing ExitCoder by printing their
+// message and calling OsExiter with the given exit code.
+//
+// If the given error instead implements MultiError, each error will be checked
+// for the ExitCoder interface, and OsExiter will be called with the last exit
+// code found, or exit code 1 if no ExitCoder is found.
+//
+// This function is the default error-handling behavior for an App.
 func HandleExitCoder(err error) {
 	if err == nil {
 		return
diff --git a/errors_test.go b/errors_test.go
index 9ed3be3..d0b1b4f 100644
--- a/errors_test.go
+++ b/errors_test.go
@@ -67,6 +67,26 @@ func TestHandleExitCoder_MultiErrorWithExitCoder(t *testing.T) {
 	expect(t, called, true)
 }
 
+func TestHandleExitCoder_MultiErrorWithoutExitCoder(t *testing.T) {
+	exitCode := 0
+	called := false
+
+	OsExiter = func(rc int) {
+		if !called {
+			exitCode = rc
+			called = true
+		}
+	}
+
+	defer func() { OsExiter = fakeOsExiter }()
+
+	err := newMultiError(errors.New("wowsa"), errors.New("egad"))
+	HandleExitCoder(err)
+
+	expect(t, exitCode, 1)
+	expect(t, called, true)
+}
+
 // make a stub to not import pkg/errors
 type ErrorWithFormat struct {
 	error