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

Adjusting cobra help text #3465

Merged
merged 1 commit into from
Jan 24, 2022
Merged

Adjusting cobra help text #3465

merged 1 commit into from
Jan 24, 2022

Conversation

Deflaimun
Copy link
Contributor

@Deflaimun Deflaimun commented Jan 13, 2022

We created a python script that autogen docs for rpk based on the texts retrieved from rpk [command] -h generated by Cobra.
This script is living in a private tooling repo for now.

This PR adapts some texts in Cobra CLI to better reflect the expected output in the docs and to avoid unexpected results.

Copy link
Contributor

@0x5d 0x5d left a comment

Choose a reason for hiding this comment

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

Thanks so much for this! I left a couple small requests.

import re

class Flag:
def __init__(self, value, type,explanation):
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's run a linter on this to fix these small formatting inconsistencies. There's already a yapf config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

def get_explanation(process_line):
return process_line[:process_line.find("Usage")].rstrip("\n")

def get_usage(process_line):
Copy link
Contributor

Choose a reason for hiding this comment

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

For each function which receives a line and parses it, could you please add comments about the structure of the line it expects, and the output?

@andrewhsu
Copy link
Member

The python script src/python/gen-rpk-help-doc.py seems like it contains html specific for what we would like to display on the documentation site. Keeping that code in this repo strongly ties the documentation formatting to this site. Not sure if this repo is the best place for that type of script. What if we want to add CSS or javascript to that html output? That script may be better suited for a repo that has a pipeline built for docs generation. (my opinion, but willing to discuss with the docs team if there is a consideration i'm missing).

The changes in the golang files in this PR to fix capitalization and whitespacing LGTM.

@andrewhsu
Copy link
Member

In addition, because rpk is using cobra, there are ways to generate man and markdown docs using the cobra golang package: https://github.com/spf13/cobra/blob/v1.3.0/doc/md_docs.md

An example of using cobra to generate docs for the GitHub CLI golang project can be found here: https://github.com/cli/cli/blob/trunk/cmd/gen-docs/main.go

Using cobra's doc generation may be easier than trying to line up --help whitespace output with expectations in a python script.

@Deflaimun
Copy link
Contributor Author

In addition, because rpk is using cobra, there are ways to generate man and markdown docs using the cobra golang package: https://github.com/spf13/cobra/blob/v1.3.0/doc/md_docs.md

An example of using cobra to generate docs for the GitHub CLI golang project can be found here: https://github.com/cli/cli/blob/trunk/cmd/gen-docs/main.go

Using cobra's doc generation may be easier than trying to line up --help whitespace output with expectations in a python script.

Yeah, we considered it in our original ticket
https://github.com/vectorizedio/documentation/issues/25
but didn't went forward with it because the output is somewhat limiting.
Writing a script gives us the flexibility to write into any template that we like.

@twmb
Copy link
Contributor

twmb commented Jan 14, 2022

We could have a hidden help flag, --help-format that accepts the format. Cobra's docs are generated using Go templates (https://pkg.go.dev/text/template) so the output format is highly customizable. Parsing the current output may be stable, but I'm not sure if we want to change things or not, whereas a --help-format would allow you to programmatically control the output. What do you think?

@Deflaimun
Copy link
Contributor Author

After discussion in our slack channel, this script won't be integrated into the pipeline. I'll be removing the .md file from this PR in result of that.

@Deflaimun
Copy link
Contributor Author

Deflaimun commented Jan 17, 2022

Addressing the reported issues (formatting) as well.
Also, moving the .py file for the doc repo.
https://github.com/vectorizedio/www/pull/444

Docs PR:
#3529

For this PR we will only keep the change to the .go files

@Deflaimun
Copy link
Contributor Author

@twmb since the output won't be directly integrated into the pipeline anymore, I guess there's not much reason to change the script now. Thank you for looking it up tho!

A comma-delimited list of the addresses (<host:port>) of all the redpanda nodes
in a cluster. The port must be the one configured for the nodes' admin API
(%d by default)`,
fmt.Sprintf(`A comma-delimited list of the addresses (<host>:<port>) of all the redpanda nodes in a cluster. The port must be the one configured for the nodes' admin API (%d by default)`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with this change (and may shorten the wording a bit later), but why merge in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the script that I wrote try to read each line, this extra was giving me some trouble. So it's a good idea to standardize it now.

cmd.Flags().StringVar(&toFile, "to-file", "", "seek to offsets as specified in the file")
cmd.Flags().StringArrayVar(&topics, "topics", nil, "only seek these topics, if any are specified")
cmd.Flags().BoolVar(&allowNewTopics, "allow-new-topics", false, "allow seeking to new topics not currently consumed (implied with --to-group or --to-file)")
cmd.Flags().StringVar(&to, "to", "", "Where to seek (start, end, unix second | millisecond | nanosecond)")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with this, and I think we should standardize around capitlizing the first word in short help text lines. I previously thought that short help (the text after the flag itself) was lowercased usually, but looking at ls --help and rg --help, first-word-capitalization seems to be common.

@Deflaimun
Copy link
Contributor Author

@twmb All points addressed.

@Deflaimun Deflaimun changed the title Create script to autogen docs for rpk Adjusting cobra help text Jan 24, 2022
@0x5d 0x5d merged commit 8712ad3 into redpanda-data:dev Jan 24, 2022
@Deflaimun Deflaimun deleted the rpk-help2 branch January 24, 2022 18:33
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants