Wrapper for long lines in help #770
Conversation
Cerebrovinny
changed the title
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily enhance the command-line interface functionality within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
internal/tui/templates/term/term_writer.go (3)
11-22: Consider enhancing the documentation with width constants explanation.The type documentation is good, but adding comments for the width constants would help future maintainers understand the reasoning behind these specific values.
const ( + // maxWidth is the maximum allowed line width, suitable for wide terminals maxWidth = 120 + // mediumWidth is the standard terminal width mediumWidth = 100 + // minWidth is the minimum preferred width before falling back to actual terminal width minWidth = 80 )
24-59: Consider wrapping errors for better debugging context.The constructor's logic is solid, but error handling could be enhanced by wrapping the terminal size error with additional context.
width, _, err := term.GetSize(int(file.Fd())) if err != nil { - return w + return w // Consider: fmt.Errorf("failed to get terminal size: %w", err) }
61-79: LGTM! Consider clarifying the io.Writer contract implementation.The Write method correctly implements the io.Writer interface, particularly in maintaining the original length return value. The implementation is clean and handles edge cases well.
Consider adding a more detailed comment about the io.Writer contract:
- // Preserving the original length for correct return value + // io.Writer contract requires returning the length of p regardless of + // how many bytes were actually written to the underlying writer originalLen := len(p)internal/tui/templates/help_printer.go (2)
19-38: Consider enhancing error handling in the constructor.While the constructor works well, it could benefit from explicit error handling when terminal width detection fails.
Consider this enhancement:
-func NewHelpFlagPrinter(out io.Writer, wrapLimit uint) *HelpFlagPrinter { +func NewHelpFlagPrinter(out io.Writer, wrapLimit uint) (*HelpFlagPrinter, error) { termWriter := term.NewResponsiveWriter(out) // Get the actual terminal width from the writer if tw, ok := termWriter.(*term.TerminalWriter); ok { - wrapLimit = tw.GetWidth() + width, err := tw.GetWidth() + if err != nil { + return nil, fmt.Errorf("failed to get terminal width: %w", err) + } + wrapLimit = width } else if wrapLimit < minWidth { wrapLimit = minWidth } - return &HelpFlagPrinter{ + return &HelpFlagPrinter{ wrapLimit: wrapLimit, out: termWriter, - } + }, nil }
62-78: Consider optimizing string operations for better performance.While the wrapping logic works correctly, we can optimize the string operations for better performance, especially when handling many flags.
Consider these optimizations:
+ var sb strings.Builder wrapped := wordwrap.WrapString(description, uint(descWidth)) lines := strings.Split(wrapped, "\n") - fmt.Fprintf(p.out, "%s%s%s\n", - prefix.String(), - strings.Repeat(" ", 4), - lines[0]) + sb.WriteString(prefix.String()) + sb.WriteString(" ") + sb.WriteString(lines[0]) + sb.WriteRune('\n') if len(lines) > 1 { indent := strings.Repeat(" ", prefixLen+4) for _, line := range lines[1:] { - fmt.Fprintf(p.out, "%s%s\n", indent, line) + sb.WriteString(indent) + sb.WriteString(line) + sb.WriteRune('\n') } } - fmt.Fprintln(p.out) + sb.WriteRune('\n') + _, _ = p.out.Write([]byte(sb.String()))internal/tui/templates/templater.go (2)
12-29: Consider enhancing documentation and parameter usage.While the implementation is solid, there are two areas for improvement:
- The Templater struct's documentation could be more detailed about its purpose and usage.
- The
b *boa.Boaparameter inSetCustomUsageFuncis currently unused.Consider adding more detailed documentation:
-// Templater handles the generation and management of command usage templates. +// Templater handles the generation and management of command usage templates. +// It provides a structured way to customize how command usage information is +// displayed to users in the CLI, ensuring consistent formatting and presentation.Also, if the
boaparameter isn't needed, consider removing it:-func SetCustomUsageFunc(cmd *cobra.Command, b *boa.Boa) error { +func SetCustomUsageFunc(cmd *cobra.Command) error {
31-57: Consider modularizing the template definition.The template is comprehensive but could benefit from better maintainability.
Consider these improvements:
- Break down the template into smaller, reusable components:
var ( usageTemplate = `Usage:{{if .Runnable}}...{{end}}` aliasTemplate = `Aliases:...` // ... more components ) func MainUsageTemplate() string { return strings.Join([]string{ usageTemplate, aliasTemplate, // ... more components }, "\n\n") }
- Add template validation to catch syntax errors early:
func validateTemplate(tmpl string) error { _, err := template.New("test").Parse(tmpl) return err }cmd/root.go (1)
111-121: Consider adding error handling, brave developer!While the template function integration is solid, the
SetCustomUsageFunccall could benefit from error handling to maintain robustness.Consider updating the code like this:
// Set custom usage template -templates.SetCustomUsageFunc(RootCmd, nil) +if err := templates.SetCustomUsageFunc(RootCmd, nil); err != nil { + u.LogErrorAndExit(schema.CliConfiguration{}, fmt.Errorf("failed to set custom usage template: %w", err)) +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
cmd/root.go(3 hunks)go.mod(3 hunks)internal/tui/templates/help_printer.go(1 hunks)internal/tui/templates/templater.go(1 hunks)internal/tui/templates/term/term_writer.go(1 hunks)
🔇 Additional comments (10)
internal/tui/templates/term/term_writer.go (2)
3-9: Well-chosen dependencies!
The imports demonstrate good choices for terminal handling and text wrapping functionality. Using golang.org/x/term for terminal operations and mitchellh/go-wordwrap for text wrapping is a solid approach.
1-79: Consider adding unit tests for edge cases.
The implementation looks solid! To ensure reliability, consider adding unit tests for:
- Non-terminal writer cases
- Terminal size detection failures
- Various width scenarios (max, medium, min, custom)
- Zero-width edge case
Let's check if tests exist:
Would you like me to help generate a test suite for this implementation?
internal/tui/templates/help_printer.go (2)
1-17: LGTM! Well-structured imports and sensible constants.
The package structure and constant definitions provide a solid foundation for the help text formatting functionality.
40-60: LGTM! Well-structured flag formatting logic.
The method handles all flag variations (shorthand, long form, boolean, non-boolean) with proper formatting and default value handling.
internal/tui/templates/templater.go (1)
1-10: LGTM! Clean package structure with focused imports.
The package structure is well-organized with clear dependency choices for CLI functionality.
cmd/root.go (3)
14-14: LGTM! Clean import addition, warrior!
The templates package import is well-placed and aligns perfectly with the PR's objective to enhance help text formatting.
Line range hint 26-37: Well-crafted battle strategy for help handling!
The PersistentPreRun implementation shows excellent attention to detail:
- Properly detects both help command and help flag
- Intelligently toggles silence behavior
- Clear and maintainable logic
139-141: Verify the necessity of additional Usage call.
The command.Usage() call after b.HelpFunc might be redundant as the help function typically handles usage display already.
Let's check how other commands handle this:
go.mod (2)
33-33: Dependencies align well with the PR objectives!
The new dependencies support the terminal-aware text wrapping system:
go-wordwrap: Provides efficient text wrapping capabilitiesgolang.org/x/term: Enables terminal width detection
These additions are precisely what we need for implementing responsive CLI help formatting.
Also applies to: 45-45
245-245: System dependency update looks good!
The update to golang.org/x/sys v0.27.0 is a stable release and maintains compatibility with the new terminal features.
|
Can we align the columns
|
columns should be aligned now @osterman
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
internal/tui/templates/help_printer.go (2)
13-15: Consider reducing the minWidth constant for better compatibility.The current minWidth of 80 characters might be too wide for some terminal windows. Consider reducing it to 60 characters to better support:
- Mobile terminals
- Split terminal windows
- Smaller viewport sizes
const ( defaultOffset = 10 - minWidth = 80 + minWidth = 60 flagIndent = " " )
32-51: Enhance helper function clarity and maintainability.The calculateMaxFlagLength function works well but could be improved:
- Extract the padding value (4) as a constant
- Add documentation explaining the length calculation
+const flagLengthPadding = 4 + +// calculateMaxFlagLength determines the maximum length needed to display any flag +// in the FlagSet. It accounts for: +// - The initial indent +// - Short flag format (-x) +// - Long flag format (--xxx) +// - Type information for non-boolean flags +// - Additional padding for alignment func calculateMaxFlagLength(flags *pflag.FlagSet) int { maxLen := 0 flags.VisitAll(func(flag *pflag.Flag) { length := len(flagIndent) if len(flag.Shorthand) > 0 { length += len(fmt.Sprintf("-%s, --%s", flag.Shorthand, flag.Name)) } else { length += len(fmt.Sprintf(" --%s", flag.Name)) } if flag.Value.Type() != "bool" { length += len(fmt.Sprintf(" %s", flag.Value.Type())) } if length > maxLen { maxLen = length } }) - return maxLen + 4 + return maxLen + flagLengthPadding }internal/tui/templates/templater.go (2)
12-15: Consider enhancing struct documentation, warrior!While the struct itself is well-documented, the
UsageTemplatefield would benefit from a description of its purpose and expected format.// Templater handles the generation and management of command usage templates. type Templater struct { + // UsageTemplate defines the format string used to generate command usage output. + // It supports cobra's template syntax for command documentation. UsageTemplate string }
32-57: Consider improving template maintainability, noble template crafter!While the template is comprehensive, breaking it into smaller, named constants would improve maintainability and make updates easier.
+const ( + usageSection = `Usage:{{if .Runnable}} + {{.UseLine}}{{end}}{{if .HasAvailableSubCommands}} + {{.CommandPath}} [command]{{end}}` + + aliasesSection = `{{if gt (len .Aliases) 0}} + +Aliases: + {{.NameAndAliases}}{{end}}` + // ... other sections ... +) + func MainUsageTemplate() string { - return `Usage:{{if .Runnable}} - {{.UseLine}}{{end}}{{if .HasAvailableSubCommands}} - {{.CommandPath}} [command]{{end}}...` + return strings.Join([]string{ + usageSection, + aliasesSection, + // ... other sections ... + }, "") }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
internal/tui/templates/help_printer.go(1 hunks)internal/tui/templates/templater.go(1 hunks)
🔇 Additional comments (2)
internal/tui/templates/help_printer.go (1)
1-91: Overall implementation looks solid! 💪
The code successfully implements terminal-aware text wrapping for CLI help output, aligning perfectly with the PR objectives. The structure is clean, and the logic is sound. While I've suggested some improvements above, they're enhancements rather than blockers.
internal/tui/templates/templater.go (1)
63-74: Verify HelpFlagPrinter implementation, fellow code warrior!
Let's verify the implementation of NewHelpFlagPrinter to ensure it handles flag formatting correctly.
✅ Verification successful
HelpFlagPrinter implementation is battle-ready, warrior!
The implementation is solid and well-structured:
- Properly handles both short and long flag formats
- Correctly calculates spacing and indentation
- Handles flag types and default values
- Implements word wrapping for descriptions
- Uses consistent formatting with proper alignment
The lack of tests is not ideal, but the implementation itself is robust and follows good practices. The NewHelpFlagPrinter constructor and its usage in WrappedFlagUsages are correctly integrated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NewHelpFlagPrinter implementation
echo "Searching for HelpFlagPrinter implementation..."
rg -t go "type.*HelpFlagPrinter.*struct"
rg -t go "func.*NewHelpFlagPrinter.*"
# Search for any tests covering this functionality
echo "Searching for related tests..."
rg -t go "Test.*HelpFlagPrinter.*"
Length of output: 499
Script:
#!/bin/bash
# Let's examine the HelpFlagPrinter implementation
echo "=== HelpFlagPrinter Implementation ==="
cat internal/tui/templates/help_printer.go
# Check for any test files in the same directory
echo -e "\n=== Related Test Files ==="
fd -e go test internal/tui/templates/
Length of output: 2327
|
@Cerebrovinny let's not put the type (string) into it's own column because it uses up a lot of white space. Put the type in the same column as the argument. |
implemented this now |
|
These changes were released in v1.102.0. |
what
why
references
This PR is also inspired from these PR/issues below:
Before:
After:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These updates aim to enhance user experience by providing clearer command usage and more robust error management.