fix[cli]: format long subcommand descriptions #3227

KuthumiPepple · 2022-04-21T14:48:44Z

Closes #2318
@squakez This is a draft PR for now. I would like to hear your thoughts on my plan to tackle this issue, to know if I'm on the right track. So I think the problem comes from the usage template string defined in the cobra package. Specifically this: {{.LocalFlags.FlagUsages}}. The returned string is not wrapped and so when a line break occurs, the output written to the terminal is not in order. My plan is to replace the .LocalFlags.FlagUsages with a template function that calls the cmd.Flags().FlagUsagesWrapped function. As you suggested previously, I'd need to get the terminal size. I will probably use the size to set the flag description length. What do you think am I on the right track? If not what suggestions could you give me?

Release Note

fix(cli): format long subcommand descriptions

squakez · 2022-04-21T14:54:55Z

Thanks for the work. I think it could be a way to fix it. You can proceed, make some experiment and post the result of any --help to see how it looks like.

KuthumiPepple · 2022-04-22T01:03:03Z

So I have implemented the fix. What do you think?

make some experiment and post the result of any --help to see how it looks like.

Ok, here is an image of the result of running kamel run --help

squakez

Looks preetty!

Let's remove the generated.deepcopy files before merging, though.

squakez · 2022-04-22T07:37:51Z

go.mod

@@ -19,6 +19,7 @@ require (
 	github.com/magiconair/properties v1.8.6
 	github.com/mitchellh/go-homedir v1.1.0
 	github.com/mitchellh/mapstructure v1.4.3
+	github.com/moby/term v0.0.0-20210619224110-3f7ff695adc6


Any reason why not to use golang.org/x/term instead?

No good reason. It was just the first package I saw while searching for a way to get the terminal size. I'll change it to golang.org/x/term

Looks preetty!

It sure does

Let's remove the generated.deepcopy files before merging, though.

Ok

KuthumiPepple · 2022-04-22T08:43:06Z

Before this is merged, I'll suggest we move this lengthy description to the more appropriate Long field and use a shorter description (perhaps the first sentence alone) for this Short field.

squakez · 2022-04-22T09:01:37Z

Before this is merged, I'll suggest we move this lengthy description to the more appropriate Long field and use a shorter description (perhaps the first sentence alone) for this Short field.

Sure, feel free to add a commit to fix it.

KuthumiPepple · 2022-04-22T09:06:44Z

Sure, feel free to add a commit to fix it.

Ok I will

KuthumiPepple · 2022-04-22T10:59:36Z

Sure, feel free to add a commit to fix it.

I've added a commit for this. You can go ahead and merge

KuthumiPepple · 2022-04-26T00:58:51Z

Hi, it's been 4 days since this contribution was approved, is there a reason why it hasn't been merged yet? I also noticed that nothing has been merged recently. OK, this is a small wall of text. I have been thinking about the current solution and I do have a suggestion for an improvement but I don't know if making any further changes would disqualify this contribution from been considered / evaluated by the mentors (I am an applicant for the Outreachy internship and have already submitted this fix as one of my contributions). Now, as to my suggestion, although the current solution works for us, I think it affects program readability (especially the replacement I did on the template string). Without adding some comments, a programmer viewing the code may find it a bit difficult to understand why I replaced that part of the string. Another problem is that future modifications/customization to the template string will not be straightforward. To eradicate these two issues, I'd suggest that in our project code, I define the usage template string customized with the desired changes by default, so we can all see the template and easily modify it in the future when needed. What do you think?

KuthumiPepple · 2022-04-26T15:51:49Z

Turns out making further updates isn't a problem. I've implemented the changes I suggested in the previous comment.
Here is an image of the result of running kamel install --help:

KuthumiPepple · 2022-04-27T08:08:20Z

@oscerd @tadayosi could either of you please review my latest changes. Squakez seems to be busy atm.

squakez · 2022-04-27T09:01:48Z

@KuthumiPepple we may have several things on going, so please, wait for some time before pushing for an answer. I will have a look as soon as possible. In the while, please, have a look at the checks failure to understand if they are related to your changes.

squakez · 2022-04-27T14:28:28Z

I had approved already, but I had a look at this last commit b573c5b after the approval and I am not sure why we want to expose the whole template. Can you please clarify the goal behind this last change? Thanks.

KuthumiPepple · 2022-04-27T14:49:46Z

I had approved already, but I had a look at this last commit b573c5b after the approval and I am not sure why we want to expose the whole template. Can you please clarify the goal behind this last change? Thanks.

The goal is to make modifications (both replacements and additions) to the template easier. Like in this case I no longer have to retrieve, replace and reset the template string just to make a modification. All I had to do was modify the needed area in the template. Without exposing the template, future alterations (especially additions) to it would be cumbersome. I also couldn't think of any security concerns not to do so but I may be wrong.

squakez · 2022-04-27T15:18:51Z

Thanks @KuthumiPepple. What I am concerned about is the presence of sections we are not using. Could we maybe have it a shorter template version to only consider usage, flags, global flags instead?

KuthumiPepple · 2022-04-27T15:43:21Z

I'm not sure I fully understand what you mean. Are you talking about having a shorter usage template for all the commands or for this specific issue? This most recent change sets the usage template for kamel and its sub-commands. We are no longer using the default. We are defining ours, hence the reason for the other sections like "Aliases" and "Available commands".

Edit: If you are talking about all the commands then sure we can cut some out but I'd have to look through each command and ensure that the associated fields are not set.

squakez

That's my point. As we are defining a template, I suggest we only cover what we need. We may introduce (and maintain) any further section when we eventually need it.

squakez · 2022-04-27T15:52:33Z

pkg/cmd/root.go

+  {{.UseLine}}{{end}}{{if .HasAvailableSubCommands}}
+  {{.CommandPath}} [command]{{end}}{{if gt (len .Aliases) 0}}
+
+Aliases:


We don't use aliases, I'd remove this section

squakez · 2022-04-27T15:52:47Z

pkg/cmd/root.go

+Aliases:
+  {{.NameAndAliases}}{{end}}{{if .HasExample}}
+
+Examples:


We don't use examples, I'd remove this section

squakez · 2022-04-27T15:54:09Z

pkg/cmd/root.go

+Global Flags:
+{{.InheritedFlags.FlagUsages | trimTrailingWhitespaces}}{{end}}{{if .HasHelpSubCommands}}
+
+Additional help topics:{{range .Commands}}{{if .IsAdditionalHelpTopicCommand}}


I'd remove this section as well

KuthumiPepple · 2022-04-27T16:33:10Z

The suggested change has been implemented.

squakez · 2022-04-28T07:36:42Z

Failures unrelated to this change

KuthumiPepple force-pushed the Issue-2318-cli-format branch from 47c34c7 to 36054f0 Compare April 22, 2022 00:35

KuthumiPepple marked this pull request as ready for review April 22, 2022 00:35

KuthumiPepple force-pushed the Issue-2318-cli-format branch from 36054f0 to 8594cfc Compare April 22, 2022 00:41

squakez reviewed Apr 22, 2022

View reviewed changes

KuthumiPepple force-pushed the Issue-2318-cli-format branch from 8594cfc to cfeaae6 Compare April 22, 2022 08:22

squakez approved these changes Apr 22, 2022

View reviewed changes

KuthumiPepple force-pushed the Issue-2318-cli-format branch from cfeaae6 to ee15d95 Compare April 22, 2022 10:55

squakez approved these changes Apr 22, 2022

View reviewed changes

KuthumiPepple force-pushed the Issue-2318-cli-format branch from c0eba3e to b573c5b Compare April 26, 2022 15:35

KuthumiPepple requested a review from squakez April 26, 2022 15:53

squakez reviewed Apr 27, 2022

View reviewed changes

KuthumiPepple added 4 commits April 27, 2022 18:01

fix(cli): format long subcommand descriptions

ba674a7

edit long and short descriptions of bind cmd

6ad65ea

define usage template and improve readability

62b7e93

remove unused portions of template

256be07

KuthumiPepple force-pushed the Issue-2318-cli-format branch from 62dd3bb to 256be07 Compare April 27, 2022 17:03

squakez approved these changes Apr 28, 2022

View reviewed changes

squakez merged commit b0e252e into apache:main Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix[cli]: format long subcommand descriptions #3227

fix[cli]: format long subcommand descriptions #3227

KuthumiPepple commented Apr 21, 2022 •

edited by squakez

Loading

squakez commented Apr 21, 2022

KuthumiPepple commented Apr 22, 2022

squakez left a comment

squakez Apr 22, 2022

KuthumiPepple Apr 22, 2022

KuthumiPepple commented Apr 22, 2022

squakez commented Apr 22, 2022

KuthumiPepple commented Apr 22, 2022

KuthumiPepple commented Apr 22, 2022

KuthumiPepple commented Apr 26, 2022 •

edited

Loading

KuthumiPepple commented Apr 26, 2022

KuthumiPepple commented Apr 27, 2022 •

edited

Loading

squakez commented Apr 27, 2022

squakez commented Apr 27, 2022

KuthumiPepple commented Apr 27, 2022 •

edited

Loading

squakez commented Apr 27, 2022

KuthumiPepple commented Apr 27, 2022 •

edited

Loading

squakez left a comment

squakez Apr 27, 2022

KuthumiPepple Apr 27, 2022

squakez Apr 27, 2022

KuthumiPepple Apr 27, 2022

squakez Apr 27, 2022

KuthumiPepple Apr 27, 2022

KuthumiPepple commented Apr 27, 2022

squakez commented Apr 28, 2022

fix[cli]: format long subcommand descriptions #3227

fix[cli]: format long subcommand descriptions #3227

Conversation

KuthumiPepple commented Apr 21, 2022 • edited by squakez Loading

squakez commented Apr 21, 2022

KuthumiPepple commented Apr 22, 2022

squakez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KuthumiPepple commented Apr 22, 2022

squakez commented Apr 22, 2022

KuthumiPepple commented Apr 22, 2022

KuthumiPepple commented Apr 22, 2022

KuthumiPepple commented Apr 26, 2022 • edited Loading

KuthumiPepple commented Apr 26, 2022

KuthumiPepple commented Apr 27, 2022 • edited Loading

squakez commented Apr 27, 2022

squakez commented Apr 27, 2022

KuthumiPepple commented Apr 27, 2022 • edited Loading

squakez commented Apr 27, 2022

KuthumiPepple commented Apr 27, 2022 • edited Loading

squakez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KuthumiPepple commented Apr 27, 2022

squakez commented Apr 28, 2022

KuthumiPepple commented Apr 21, 2022 •

edited by squakez

Loading

KuthumiPepple commented Apr 26, 2022 •

edited

Loading

KuthumiPepple commented Apr 27, 2022 •

edited

Loading

KuthumiPepple commented Apr 27, 2022 •

edited

Loading

KuthumiPepple commented Apr 27, 2022 •

edited

Loading