Add man pages generation utility, include in packaging#42
Add man pages generation utility, include in packaging#42austin-denoble merged 10 commits intomainfrom
man pages generation utility, include in packaging#42Conversation
…ly only when color is enabled in the client, strip out ANSI for man page rendering
|
Claude finished @austin-denoble's task —— View job PR Review: Add generation utility for man pagesI've completed a comprehensive review of this pull request. Overall, this is a well-structured implementation for generating man pages with good separation of concerns. Review Tasks:
✅ Strengths
🐛 Potential Issues
🔒 Security Assessment
⚡ Performance Considerations
🧪 Test Coverage AssessmentStrengths:
Gaps:
📝 Suggestions for Improvement
🎯 Minor Code Quality Notes
Overall Assessment: This is a solid implementation that integrates well with the existing codebase. The approach of leveraging Cobra's built-in man page generation while handling the color configuration elegantly is well thought out. The code is production-ready with only minor suggestions for enhancement. |
|
Claude finished @austin-denoble's task —— View job PR Review: Add generation utility for man pagesReview Tasks:
I've completed a comprehensive review of this pull request. Overall, this is a well-structured implementation for generating man pages with good separation of concerns. ✅ Strengths
🐛 Potential Issues
🔒 Security Assessment
⚡ Performance Considerations
🧪 Test Coverage AssessmentStrengths:
Gaps:
📝 Suggestions for Improvement
🎯 Minor Code Quality Notes
Overall Assessment: This is a solid implementation that integrates well with the existing codebase. The approach of leveraging Cobra's built-in man page generation while handling the color configuration elegantly is well thought out. The code is production-ready with only minor suggestions for enhancement. |
…ily at the callsite, and sanitizing similar to Example. remove ANSI injections and word wrapping from the help text strings before they're passed in to avoid applying to man/groff files. add terminal-aware wrapping for Long/Short Command strings at runtime and a renderLongBlock fn to render.go. update HelpTemplate to display Short or Long descriptions at the top, and apply the pcBlock rendering func in EnableHelpRendering
|
Claude finished @austin-denoble's task —— View job PR Review: Add generation utility for man pagesReview Tasks:
I've completed a comprehensive review of this pull request. Overall, this is a well-structured implementation for generating man pages with good separation of concerns. ✅ Strengths
🐛 Potential Issues
🔒 Security Assessment
⚡ Performance Considerations
🧪 Test Coverage AssessmentStrengths:
Gaps:
📝 Suggestions for Improvement
🎯 Code Quality NotesExcellent:
Minor Improvements:
🔍 Implementation Details Review
Overall Assessment: This is a solid, production-ready implementation that integrates seamlessly with the existing codebase. The approach of leveraging Cobra's built-in man page generation while handling color configuration elegantly is well thought out. The code demonstrates good understanding of both the domain (CLI help systems) and Go best practices. Recommendation: ✅ Approve - Ready to merge with only minor suggestions for future enhancement. |
…est validating Examples coloring since that moved to the render path
|
Claude finished @austin-denoble's task —— View job Claude is reviewing the PR PR Review: Add generation utility for man pagesReview Tasks:
I've completed a comprehensive review of this pull request. Overall, this is a well-structured implementation for generating man pages with good separation of concerns. ✅ Strengths
🐛 Potential Issues
🔒 Security Assessment
⚡ Performance Considerations
🧪 Test Coverage AssessmentStrengths:
Gaps:
📝 Suggestions for Improvement
🎯 Code Quality NotesExcellent:
Minor Improvements:
🔍 Implementation Details Review
Overall Assessment: This is a solid, production-ready implementation that integrates seamlessly with the existing codebase. The approach of leveraging Cobra's built-in man page generation while handling color configuration elegantly is well thought out. The code demonstrates good understanding of both the domain (CLI help systems) and Go best practices. Recommendation: ✅ Approve - Ready to merge with only minor suggestions for future enhancement. |
…all them via brew
…both pc and pinecone
|
Claude finished @austin-denoble's task —— View job PR Review: Add generation utility for man pagesReview Tasks:
I've completed a comprehensive review of this pull request. Overall, this is a well-structured implementation for generating man pages with good separation of concerns. ✅ Strengths
🐛 Potential Issues
🔒 Security Assessment
⚡ Performance Considerations
🧪 Test Coverage AssessmentStrengths:
Gaps:
📝 Suggestions for Improvement
🎯 Code Quality NotesExcellent:
Minor Improvements:
🔍 Implementation Details Review
Overall Assessment: This is a solid, production-ready implementation that integrates seamlessly with the existing codebase. The approach of leveraging Cobra's built-in man page generation while handling color configuration elegantly is well thought out. The code demonstrates good understanding of both the domain (CLI help systems) and Go best practices. Recommendation: ✅ Approve - Ready to merge with only minor suggestions for future enhancement. |
|
Claude finished @austin-denoble's task —— View job Claude is reviewing the PR PR Review: Add generation utility for man pagesReview Tasks:
I've completed a comprehensive review of this pull request. Overall, this is a well-structured implementation for generating man pages with good separation of concerns. ✅ Strengths
🐛 Potential Issues
🔒 Security Assessment
⚡ Performance Considerations
🧪 Test Coverage AssessmentStrengths:
Gaps:
📝 Suggestions for Improvement
🎯 Code Quality NotesExcellent:
Minor Improvements:
🔍 Implementation Details Review
Overall Assessment: This is a solid, production-ready implementation that integrates seamlessly with the existing codebase. The approach of leveraging Cobra's built-in man page generation while handling color configuration elegantly is well thought out. The code demonstrates good understanding of both the domain (CLI help systems) and Go best practices. Recommendation: ✅ Approve - Ready to merge with only minor suggestions for future enhancement. |
man pages generation utility, include in packaging
| install: | | ||
| bin.install "pc" | ||
| bin.install_symlink "pc" => "pinecone" | ||
|
|
There was a problem hiding this comment.
This installs the man pages as a part of the brew installation process. I think what I have here should work for linking both aliases to the pages, but might be worth taking a look at if people know the brew / ruby syntax better than I.
| "github.com/pinecone-io/cli/internal/pkg/utils/manpage" | ||
| ) | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
Go command wrapper for running the manpage.GenerateManPages utility.
| Long: collectionHelpText, | ||
| Use: "collection <command>", | ||
| Short: "Work with collections", | ||
| Long: help.Long(` |
There was a problem hiding this comment.
The way I did the normalization in help.Long is meant to give us a bit more flexibility when defining the documentation at the callsite.
|
Claude finished @austin-denoble's task —— View job PR Review: Add generation utility for man pagesReview Tasks:
I've completed a comprehensive review of this pull request. Overall, this is a well-structured implementation for generating man pages with good separation of concerns. ✅ Strengths
🐛 Potential Issues
🔒 Security Assessment
⚡ Performance Considerations
🧪 Test Coverage AssessmentStrengths:
Gaps:
📝 Suggestions for Improvement
🎯 Code Quality NotesExcellent:
Minor Improvements:
🔍 Implementation Details Review
Overall Assessment: This is a solid, production-ready implementation that integrates seamlessly with the existing codebase. The approach of leveraging Cobra's built-in man page generation while handling color configuration elegantly is well thought out. The code demonstrates good understanding of both the domain (CLI help systems) and Go best practices. Recommendation: ✅ Approve - Ready to merge with only minor suggestions for future enhancement. |
| cobra.AddTemplateFunc("pcExamples", renderExamples) | ||
| cobra.AddTemplateFunc("pcBlock", renderLongBlock) |
There was a problem hiding this comment.
This is where these rendering functions are applied to the Cobra template. Check template.go for where these are defined.
|
|
||
| // Colors help examples with ANSI color codes only when color is enabled | ||
| // Allows stripping ANSI color codes when generating man pages | ||
| func renderExamples(s string) string { |
There was a problem hiding this comment.
This function primarily applies ANSI colors to comments and commands when rendering examples.
| return strings.Join(lines, "\n") | ||
| } | ||
|
|
||
| func renderLongBlock(s string) string { |
There was a problem hiding this comment.
This function attempts to word wrap longer comment blocks based on the terminal size if available. It also tries to preserve basic markdown syntax.
| var dot = style.Emphasis(style.Dot) | ||
|
|
||
| var part1 = fmt.Sprintf(`%s{{if .Runnable}} | ||
| var description = `{{with (or .Long .Short)}}{{pcBlock .}}{{end}} |
There was a problem hiding this comment.
I renamed these template variables to be a bit more descriptive in terms of what they contain. This description block is new and attaches the pcBlock rendering function to the available string content.
|
|
||
| %s | ||
| {{.Example}}{{end}}{{if .HasAvailableSubCommands}}{{$cmds := .Commands}}{{if eq (len .Groups) 0}} | ||
| {{pcExamples .Example}}{{end}}{{if .HasAvailableSubCommands}}{{$cmds := .Commands}}{{if eq (len .Groups) 0}} |
There was a problem hiding this comment.
Attaching the render function to the example block.
| return false | ||
| } | ||
|
|
||
| func wrap(s string, width int) string { |
There was a problem hiding this comment.
This is the function that actually handles wrapping. The functions above are mostly helpers for categorizing what type of content we're working with.
bradumbaugh
left a comment
There was a problem hiding this comment.
Hey @austin-denoble — this looks reasonable to me, but I'm not sure I know enough here to give you a good review of this approach. One question that jumped out at me is whether or not we need to give help.Long descriptions to more of the operations? Looks like currently only 5-6 of them have those.
Yup, good callout. There are currently six of them defined for
|
|
|
||
| %s | ||
| `, style.CodeWithPrompt("pc login")), | ||
| Get started by logging in with $ pc login |
There was a problem hiding this comment.
Can we format this differently so the "$ pc login" is differentiated in some way? Without color I don't think it works well inline.
| For automation use cases relying on API Keys for authentication, there's no need | ||
| to specify a project context as the API Key is already associated with a specific |
There was a problem hiding this comment.
| For automation use cases relying on API Keys for authentication, there's no need | |
| to specify a project context as the API Key is already associated with a specific | |
| For automation use cases relying on API keys for authentication, there's no need | |
| to specify a project context as the API key is already associated with a specific |
In our larger body of docs, we don't typically capitalize resource names like Index or Collection. So I don't see why we would capitalize Keys here.
There was a problem hiding this comment.
Another good catch, I had tried to un-capitalize where I could, but missed a few. 👍
| Use: "target <flags>", | ||
| Short: "Set context for the CLI", | ||
| Long: targetHelp, | ||
| Long: help.Long(` |
There was a problem hiding this comment.
Does the target command give an error if you try to use it while authing with API key?
There was a problem hiding this comment.
I don't think it does at the moment, good catch. Will address in a follow up PR with a couple other smaller things.
jhamon
left a comment
There was a problem hiding this comment.
I dropped a few nits, but overall it looks good.
## Problem @bradumbaugh had a good callout on a [previous PR](#42) around some of our commands, especially admin commands, missing more verbose help text. Areas currently lacking: - `api-key` - `project` - `organization` - `auth` - `login/logout` - `configure` - `local-keys` - `target` ## Solution I've gone through and cleaned up all of our help-text definitions to be somewhat consistent across commands. I've also added in more verbose help text for the above commands, and tweaked some of the verbiage in others. These strings show up both for `--help`, and in man pages. ## Type of Change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update - [ ] Infrastructure change (CI configs, etc) - [X] Non-code change (docs, etc) - [ ] None of the above: (explain here) ## Test Plan No functional changes, documentation changes can be seen by running with `--help` or checking man pages. --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1211580374844615
Problem
Currently, the Pinecone CLI does not ship with any kind of man pages. In general, on Unix and Unix-like systems, man pages are an expected means of documenting programs and utilities. In order to make the self-documenting nature of the CLI more robust, we should support generating man pages based on the CLI structure, and installing these pages as a part of the CLI packaging pipeline.
For an example of what these look like, you can try out running
manon a few utilities you may have installed:$ man gh $ man kubectl $ man sed # etcWhat we want to support is
man pcandman pinecone.Solution
Summary:
/cmd/gen-manpages/main.go, and/internal/pkg/utils/manpage/manpage.go).justfileto handle developer commands likejust test,just gen-manpages,just build, andjust build-all./internal/pkg/utils/help/package to improve overall experience of writing and rendering documentation:examples.go->content.go- add utilities for normalizingCommand.Longdescriptions, removing ANSI coloring fromExamplesfunc.render.gofile tohelppackage - this file contains the logic for template rendering of Example and Long fields defined in thecobra.Command{}, which allows us to apply things like ANSI colors and word wrapping at render time, rather than against the strings themselves.help.Examples()andhelp.Long()functions when defining documentation. Strip out any ANSI, and move to rendering if we still want it..goreleaser.yamlto handle building man pages as a part of the release packaging process, and installing them via brew. NOTE: The man pages will be included as a part of the archive on Git, so users who download directly rather than using a package manager will still have access to them. This can maybe be improved in the future with more tooling.Background / Context
Since we're using Cobra to build out the CLI, we can easily slot in
cobra/docto support generating man pages from our configured command tree without much additional work. Thecobra/docpackage gives us access to a number of utilities which could be expanded on later for working with different forms of CLI documentation: https://pkg.go.dev/github.com/spf13/cobra/doc.When users are installing the CLI via brew, we can hook into installing man pages for
pc(andpinecone) directly for the user so the pages are available along with the CLI installation.Cobra takes what you have defined in your various
*&cobra.Commandconfigurations. This means that command structure, flags, examples, and documentation defined here end up building up man pages, and also the help that's displayed via--helpfor any command level.An example of what this looks like in practice:
There were a few issues I ran into after generating man pages from our initial configurations. We were injecting ANSI for specific things and word wrapping everything to 80 columns before applying the strings to the
cobra.Command{}. This meant that we ended up with weird looking output in our man pages since groff (man pages format) cannot handle rendering ANSI or more complex forms of markdown.Type of Change
Test Plan
You can test the man page generation utility and the output yourself by checking out this branch, and calling
just gen-manpages. This will dump everything generated intoman/man1/pc*.1.From there, you can test what the content looks like by calling
managainst it directly:In order to test the brew packaging and distribution changes, we'll need to run a new release with these changes included.