Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify HelpPrinter and CustomHelpPrinter behaviors #912

Merged
merged 5 commits into from Oct 21, 2019

Conversation

rliebz
Copy link
Member

@rliebz rliebz commented Oct 16, 2019

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)

@rliebz rliebz requested a review from a team as a code owner October 16, 2019 11:58
@codecov
Copy link

codecov bot commented Oct 16, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

Copy link
Member

@coilysiren coilysiren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

rliebz commented Oct 17, 2019

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.

Copy link
Member

@coilysiren coilysiren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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!

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rliebz
Copy link
Member Author

rliebz commented Oct 21, 2019

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
Copy link
Member

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
Copy link
Member Author

rliebz commented Oct 21, 2019

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.

@coilysiren coilysiren merged commit 49ad3df into urfave:master Oct 21, 2019
coilysiren pushed a commit that referenced this pull request Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants