Introduce helpTopics type and reduce duplication across commands

[williammartin]

Description

As I was investigating #5073 I noticed some temporal coupling between commands. Specifically, the reference help topic (introduced in #2223) has to have its long description set after every other command is added to the root. This is required because it needs to stringify data from all the commands to serve as the reference.

This is probably a very minor maintenance burden in practice. That said, I did have to go through the project history to understand the comment "this needs to appear last" because it didn't explain why.

Since I was looking at this code, and am in exploratory mode, it seemed like something I could try to resolve and get feedback on. Think of this more as a means for me to learn, and to start some conversations, rather than because I think this is a real pressing issue to fix!

There is no associated issue to fix but I can create one if that is useful.

Update

I've updated the title and contents of this PR based on this thread highlighting some issues around the Long text being used for documentation.

This PR is now a general reworking to provide some types and comments that I would have found useful initially.

[williammartin]

This is really the difference maker, by pushing the stringification into the help func, we know that this command has been attached somewhere below the root (at least, if my understanding of cmd.Root() is correct 🌚 ) so we can walk the command tree.

Note that doing this has two consequences:

• The cmd.Long is not set (but I'm not sure if this was used except to serve as a place to store the markdown to render - THIS IS MY BIG OPEN QUESTION)
• Markdown rendering would occur each time we call the help func (but I can't think of a reason it would be called multiple times in one CLI invocation)

[samcoe]

The reason it is important to set cmd.Long is for the auto generating of our online manual. This gets generated at release time here which eventually calls this file. This is not at all obvious from the code and we have fielded questions about how the online docs are generated multiple times. We will need to figure out how to best solve this so that the online manual is not effected.

[williammartin]

Nice! This is exactly the kind of thing I was hoping to get out of this PR. I'll look into it and see what I can do.

[mislav]

Since we control the entrypoint for generation of man pages/HTML manual, which @samcoe pointed to, maybe those scripts could trigger generating a cmd.Long value for the reference command? That way, it wouldn't matter if the reference command was generated last or not.

I wish that Cobra invoked methods instead of relied on struct fields for values like these.

[williammartin]

It's kind of unfortunate because then we'd diverge on real CLI usage vs the doc generation to produce this, which is maybe more complicated than an explicit comment. Explicit and a bit janky is better than implicit and adding layers of abstraction to make it seem less janky.

I'm inclined to back out the reference specific work and just leave the structural changes in place with an additional comment on what we've discussed here. Does that sound reasonable?

[mislav]

I also don't feel like it's very important to remove the temporal coupling of the reference command if it's going to be too large of a pull, especially because there is already a comment explaining why that specific line should remain last. Feel free to reduce the scope of this work if you would.

[williammartin]

It seemed reasonable to move this to a strongly typed structure rather than a nested map of strings.

[williammartin]

I like immediately invoked function expressions to avoid mutating variables, but sometimes people don't like these at all, so let me know.

[samcoe]

I am not sure what the immediately invoked function expression buys us? We don't tend to use them. I am open to them but if you could explain their purpose a bit more that would be helpful.

[williammartin]

Oh sure. I feel 2/10 strongly about this, so it's not a big deal. There are sort of three options here:

Zero value then mutate

var helpFunc func(*cobra.Command, []string)
if ht.helpFuncFn != nil {
    helpFunc = ht.helpFuncFn(ios)
} else {
    helpFunc = func(c *cobra.Command, _ []string) {
	helpTopicHelpFunc(ios.Out, c)
   }
}

When I look at this as a maintainer, I am immediately drawn to the zero value. Does it have meaning? Is it definitely set in all branches (if there is more complicated logic than a ternary)?

Default then set

helpFunc := func(c *cobra.Command, _ []string) {
    helpTopicHelpFunc(ios.Out, c)
}

if ht.helpFuncFn != nil {
    helpFunc = ht.helpFuncFn(ios)
} 

This answers my questions about the zero value, and I now know that it's always set to some sensible default. However, in general I tend to favour setting variables exactly once and never changing them again, since when I'm reading code later, it allows for some useful mental heuristics. Additionally, I find that when things get a bit more complex, it's nice to encapsulate it all inside a function so that I know all set up for this variable belongs inside this function and I don't need to look anywhere else, hence I just default to IIFE across the board for ternary and beyond:

IIFE

helpFunc := func() func(*cobra.Command, []string) {
    if ht.helpFuncFn != nil {
        return ht.helpFuncFn(ios)
    }

    return func(c *cobra.Command, _ []string) {
	helpTopicHelpFunc(ios.Out, c)
    }
}()

Idioms

All that said, I think mutation is generally the idiomatic way in Go and since you say we don't use them, I'll just change this and commit to that style to avoid introducing extra pattern complexity.

[samcoe]

Thanks for taking the time to explain this pattern a bit more. I generally think that you will see the default and set pattern the most in our codebase.

What are your thoughts on something like this:

	helpFunc := func(c *cobra.Command, _ []string) {
		if ht.helpFuncFn != nil {
			ht.helpFuncFn(ios)
			return
		}
		helpTopicHelpFunc(ios.Out, c)
	}

Similar to default and set but all the logic is encapsulated in a function and the variable is set only once.

[williammartin]

I think that's fine too but I generally like pushing conditionals up the stack in a sense because like mutability, it helps with mental heuristics because once the conditional is evaluated once you never have to think about it again (as opposed to each time the function is called here).

It doesn't really matter in this simple case. Nice suggestion that I hadn't really thought about though!

I'm going to change this to default and set (also, nice that we have a shared language for the pattern now). Cheers.

[williammartin]

Something for discussion, providing the assertion here directly instead of a bool pushes the conditional out of the test. I've seen a lot of assert.XYZ without return in my time.

At the least, I don't think there's ever a good time to use assert.Error or assert.NoError instead of require.

[samcoe]

This makes sense to me in this use case. In other use cases would you create a slice of assertions if there were multiple assertions that need to be made?

[williammartin]

No, probably not. I think the complexity in the setup would outweigh the benefit in terms of readability. Testify suggests it can be used for comparisons, errors and panics e.g. the example for https://pkg.go.dev/github.com/stretchr/testify/assert#ComparisonAssertionFunc

Like IIFEs, I feel 2/10 strongly about it. I think it's a neat pattern and low risk of causing issues though. Easy to back out if we didn't like the results of experimenting with it.

[williammartin]

One nice thing about moving this into a slice of helpTopic is that we've reduced the amount of knowledge that existed in two places i.e. the names of the help topics themselves. Similar to my comment above, it's nice to know that if something is in the slice of HelpTopics, it will be in the command list and counter to that, there's no way to reference a meaningless help topic here.

[williammartin]

There are some things we could do about this search but I don't think it's really a big deal. It sort of depends on what we're really trying to test here. It seems like maybe the test in this file is about how the NewCmdHelpTopic function uses a helpTopic, rather than environment specifically.

In that case we could just create a helpTopic directly in the test. This would also be nice because we could inject a helpTopic that overrides the helpFunc default. I haven't done this yet just because I already had a few things I wanted to discuss.

[williammartin]

Self-reviewed with some things I want to draw attention to.

From my side, if someone has seen a comment but has nothing to add, it's nice to see 👀 emoji so I know it hasn't been missed, but this is a personal desire so no worries if that doesn't work for whoever reviews this.

[williammartin]

Self-reviewed with some things I want to draw attention to.

From my side, if someone has seen a comment but has nothing to add, it's nice to see 👀 emoji so I know it hasn't been missed, but this is a personal desire so no worries if that doesn't work for whoever reviews this.

[samcoe]

@williammartin I love the improvements here, creating the helpTopic struct is really clean. I think the one blocker is around the generating of online docs which I elaborated on in a code level comment.

[samcoe]

The reason it is important to set cmd.Long is for the auto generating of our online manual. This gets generated at release time here which eventually calls this file. This is not at all obvious from the code and we have fielded questions about how the online docs are generated multiple times. We will need to figure out how to best solve this so that the online manual is not effected.

[samcoe]

I am not sure what the immediately invoked function expression buys us? We don't tend to use them. I am open to them but if you could explain their purpose a bit more that would be helpful.

[samcoe]

This makes sense to me in this use case. In other use cases would you create a slice of assertions if there were multiple assertions that need to be made?

[mislav]

Thanks for working on this!

[mislav]

Since we control the entrypoint for generation of man pages/HTML manual, which @samcoe pointed to, maybe those scripts could trigger generating a cmd.Long value for the reference command? That way, it wouldn't matter if the reference command was generated last or not.

I wish that Cobra invoked methods instead of relied on struct fields for values like these.

[mislav]

Thank you! Looking good, and reference.Long could stay the way it was before.

Addressed these changes by reverting to using Long and reviewed with Mislav