Simplify HelpPrinter and CustomHelpPrinter behaviors

[rliebz]

Currently, the structure ofHelpPrinter and HelpPrinterCustom make them extremely difficult to use correctly.

As it stands:

• For any app configuration, HelpPrinter will be used most of the time. For most applications, it will be used 100% of the time.
• Specifically when printing app help or command help for an application when the command's CustomHelpTemplate is set, HelpPrinterCustom will be used. Any other time, HelpPrinter will continue to be used.
• While the default HelpPrinter simply makes a call to the default HelpPrinterCustom, it calls the private version of that function. This means if HelpPrinterCustom is overridden, HelpPrinter must also be set in order to get consistent behavior across the application.

As an example, in an application that needs no custom behavior other than passing one additional custom template function, the easiest way I could find to get a consistent print behavior was to first wrap the original custom help printer, then assign to both HelpPrinter and HelpPrinterCustom:

func init() {
	// These are both sometimes used, so both must be overridden
	cli.HelpPrinterCustom = wrapPrinter(cli.HelpPrinterCustom)
	cli.HelpPrinter = func(w io.Writer, templ string, data interface{}) {
		cli.HelpPrinterCustom(w, templ, data, nil)
	}
}

// helpPrinter includes the custom indent template function.
func wrapPrinter(p helpPrinterCustom) helpPrinterCustom {
	return func(w io.Writer, tpl string, data interface{}, customFuncs map[string]interface{}) {
		// The exact function used here is just an example
		customFuncs["indent"] = func(spaces int, text string) string {
			padding := strings.Repeat(" ", spaces)
			return padding + strings.ReplaceAll(text, "\n", "\n"+padding)
		}

		// And now we defer back to the original function call
		p(w, tpl, data, customFuncs)
	}
}

The fix in this PR does a few things that should make things more consistent and harder to misuse.

Since I don't think there's a good justification for the inconsistent calling of HelpPrinterCustom in one of the specific places where it happens to be used, I've replaced it with the higher-level HelpPrinter call, which by default will defer to HelpPrinterCustom. In the one place where it was actually required, the logic has been updated to prefer HelpPrinter except in the exact case which HelpPrinterCustom was written to fulfill.

As well, the default PrintHelp function now calls the public version of PrintHelpCustom, so if that is overridden, users don't also have to override PrintHelp for the change to propagate.

Combining these changes means that you can override either HelpPrinter or HelpPrinterCustom as necessary, and still get consistent behavior between printing app help, command help, and sub-command help, regardless of whether or not you're using a custom template. The default behavior is exactly the same as it was before.

I think this is the correct fix, but I figured it would be easier to get code out for discussion rather than opening an issue. Happy to make changes if need be.

---

TL;DR: HelpPrinter and HelpPrinterCustom were called interchangeably based on logic that didn't really make sense from a public interface perspective. Two changes:

• HelpPrinter now directly calls HelpPrinterCustom instead of just having the same implementation, so you don't have to override both if you override HelpPrinterCustom.
• Since HelpPrinter is just a wrapper around HelpPrinterCustom, only call HelpPrinterCustom directly in the one situation where we absolutely have to (see Add support for custom help templates. #586 for context)

[codecov]

Codecov Report

Merging #912 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #912      +/-   ##
==========================================
+ Coverage   71.38%   71.48%   +0.09%     
==========================================
  Files          30       30              
  Lines        2394     2395       +1     
==========================================
+ Hits         1709     1712       +3     
+ Misses        578      577       -1     
+ Partials      107      106       -1

Impacted Files	Coverage Δ
help.go	81.6% <100%> (+1.26%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a221e66...9ab178d. Read the comment docs.

[coilysiren]

I'm having a hard time understanding the practical impact of this change, can you write a test that helps display the new behavior?

[rliebz]

Four new test cases added that should cover everything that's changed.

I also updated the GoDoc comments and PR description to hopefully make this more intuitive in addition to the new test cases. There's now a TL;DR in the description that's a bit more direct than the wall of text above it.

[coilysiren]

👍 looking good ✨

I'm a bit hesitant to merge this since we are currently trying to fast track the V2 work, but the PR is looking great!

[saschagrunert]

LGTM

[rliebz]

If the concern with merging now is just avoiding the merge conflicts for the v2 branch, I went ahead and resolved issues ahead of time on a separate branch: https://github.com/rliebz/cli/tree/help-printer-v2 (merge commit: b5bef8e).

I don't have a good idea of what the migration path from v1 to v2 looks like, but I think there's value into getting fixes like this into v1. There are ways to work around the issue though, so if we end up not merging this in, that's alright. The current state of things is still manageable, just not ideal.

[coilysiren]

I went ahead and resolved issues ahead of time on a separate branch

🙏 ahhhh, this is hero behavior! thank you!!! 🙏

You can totally merge this now 😄 and then yea, you'll want to send a PR into the WIP v2 branch

[rliebz]

Awesome, thanks! I've opened up #913 to do just that.

I don't have permission to merge, so please do so when you get the chance.