diff --git a/e2e/cards_columns_steps.bats b/e2e/cards_columns_steps.bats index 02cf37d2..5afab4fc 100644 --- a/e2e/cards_columns_steps.bats +++ b/e2e/cards_columns_steps.bats @@ -192,7 +192,7 @@ load test_helper run basecamp cards column foobar # Cobra doesn't error on unknown args, shows help assert_success - assert_output_contains "Available Commands" + assert_output_contains "COMMANDS" } @@ -362,5 +362,5 @@ load test_helper run basecamp cards step foobar # Cobra doesn't error on unknown args, shows help assert_success - assert_output_contains "Available Commands" + assert_output_contains "COMMANDS" } diff --git a/internal/cli/help.go b/internal/cli/help.go index 09b56720..010d1747 100644 --- a/internal/cli/help.go +++ b/internal/cli/help.go @@ -6,35 +6,26 @@ import ( "strings" "github.com/spf13/cobra" + "github.com/spf13/pflag" "github.com/basecamp/basecamp-cli/internal/commands" "github.com/basecamp/basecamp-cli/internal/output" ) -// rootHelpFunc returns a help function that renders gh-style help for the root -// command, agent JSON when --agent is set, and falls through to cobra's default -// for subcommands. -func rootHelpFunc(defaultHelp func(*cobra.Command, []string)) func(*cobra.Command, []string) { +// rootHelpFunc returns a help function that renders styled help for all +// commands: agent JSON when --agent is set, curated categories for root, +// and a consistent styled layout for every subcommand. +func rootHelpFunc() func(*cobra.Command, []string) { return func(cmd *cobra.Command, args []string) { - // --agent → structured JSON help if agent, _ := cmd.Root().PersistentFlags().GetBool("agent"); agent { emitAgentHelp(cmd) return } - - // Commands with registered custom help renderers - if fn, ok := commands.CustomHelp(cmd); ok { - fn(cmd, args) + if cmd == cmd.Root() { + renderRootHelp(cmd.OutOrStdout(), cmd) return } - - // Subcommands use cobra's default help - if cmd != cmd.Root() { - defaultHelp(cmd, args) - return - } - - renderRootHelp(cmd.OutOrStdout(), cmd) + renderCommandHelp(cmd) } } @@ -186,3 +177,143 @@ func renderRootHelp(w io.Writer, cmd *cobra.Command) { fmt.Fprint(w, b.String()) } + +// renderCommandHelp renders styled help for any non-root command, reading +// structure from cobra's command tree rather than hardcoding per-command. +func renderCommandHelp(cmd *cobra.Command) { + w := cmd.OutOrStdout() + r := output.NewRenderer(w, false) + var b strings.Builder + + // Description + desc := cmd.Long + if desc == "" { + desc = cmd.Short + } + if desc != "" { + b.WriteString(desc) + b.WriteString("\n") + } + + // USAGE + b.WriteString("\n") + b.WriteString(r.Header.Render("USAGE")) + b.WriteString("\n") + if cmd.HasAvailableSubCommands() && !cmd.Runnable() { + b.WriteString(" " + cmd.CommandPath() + " [flags]\n") + } else { + b.WriteString(" " + cmd.UseLine() + "\n") + } + + // ALIASES + if len(cmd.Aliases) > 0 { + b.WriteString("\n") + b.WriteString(r.Header.Render("ALIASES")) + b.WriteString("\n") + b.WriteString(" " + cmd.Name()) + for _, a := range cmd.Aliases { + b.WriteString(", " + a) + } + b.WriteString("\n") + } + + // COMMANDS + if cmd.HasAvailableSubCommands() { + var entries []helpEntry + maxName := 0 + for _, sub := range cmd.Commands() { + if !sub.IsAvailableCommand() { + continue + } + entries = append(entries, helpEntry{name: sub.Name(), desc: sub.Short}) + if len(sub.Name()) > maxName { + maxName = len(sub.Name()) + } + } + b.WriteString("\n") + b.WriteString(r.Header.Render("COMMANDS")) + b.WriteString("\n") + for _, e := range entries { + fmt.Fprintf(&b, " %-*s %s\n", maxName, e.name, e.desc) + } + } + + // FLAGS (all local flags: persistent + non-persistent) + localFlags := cmd.LocalFlags() + localUsage := strings.TrimRight(localFlags.FlagUsages(), "\n") + if localUsage != "" { + b.WriteString("\n") + b.WriteString(r.Header.Render("FLAGS")) + b.WriteString("\n") + b.WriteString(localUsage) + b.WriteString("\n") + } + + // INHERITED FLAGS + // Parent-defined persistent flags (--project, --campfire, etc.) always + // show — they carry required context. Root-level global flags are curated + // to the essentials so leaf help isn't 20+ lines of noise. + inherited := filterInheritedFlags(cmd) + if inherited != "" { + b.WriteString("\n") + b.WriteString(r.Header.Render("INHERITED FLAGS")) + b.WriteString("\n") + b.WriteString(inherited) + b.WriteString("\n") + } + + // EXAMPLES + if cmd.Example != "" { + b.WriteString("\n") + b.WriteString(r.Header.Render("EXAMPLES")) + b.WriteString("\n") + for _, line := range strings.Split(cmd.Example, "\n") { + b.WriteString(r.Muted.Render(line) + "\n") + } + } + + // LEARN MORE + b.WriteString("\n") + b.WriteString(r.Header.Render("LEARN MORE")) + b.WriteString("\n") + if cmd.HasAvailableSubCommands() { + b.WriteString(" " + cmd.CommandPath() + " --help\n") + } else if cmd.HasParent() { + b.WriteString(" " + cmd.Parent().CommandPath() + " --help\n") + } + + fmt.Fprint(w, b.String()) +} + +// salientRootFlags is the curated set of root-level global flags shown in +// inherited flag sections. Parent-defined persistent flags always appear; +// only root globals are filtered to this set. +var salientRootFlags = map[string]bool{ + "account": true, + "json": true, + "md": true, + "project": true, + "quiet": true, +} + +// filterInheritedFlags returns formatted flag usages for inherited flags, +// keeping all parent-defined persistent flags and curating root globals +// to the salient set. Provenance is determined by pointer identity: if the +// flag object is the same pointer as the one on root's PersistentFlags, +// it truly originates from root. A parent that redefines the same name +// (e.g. --project on messages) produces a different pointer and always +// passes through. +func filterInheritedFlags(cmd *cobra.Command) string { + root := cmd.Root() + filtered := pflag.NewFlagSet("inherited", pflag.ContinueOnError) + + cmd.InheritedFlags().VisitAll(func(f *pflag.Flag) { + rootFlag := root.PersistentFlags().Lookup(f.Name) + if rootFlag != nil && rootFlag == f && !salientRootFlags[f.Name] { + return + } + filtered.AddFlag(f) + }) + + return strings.TrimRight(filtered.FlagUsages(), "\n") +} diff --git a/internal/cli/help_test.go b/internal/cli/help_test.go index 669a96cb..8d3b4fc7 100644 --- a/internal/cli/help_test.go +++ b/internal/cli/help_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" + "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -74,7 +75,7 @@ func TestRootHelpContainsLearnMore(t *testing.T) { assert.Contains(t, out, "basecamp -h") } -func TestSubcommandGetsDefaultHelp(t *testing.T) { +func TestSubcommandGetsStyledHelp(t *testing.T) { isolateHelpTest(t) var buf bytes.Buffer @@ -86,10 +87,151 @@ func TestSubcommandGetsDefaultHelp(t *testing.T) { _ = cmd.Execute() out := buf.String() - // Subcommand help should NOT have our curated categories + assert.Contains(t, out, "USAGE") + assert.Contains(t, out, "COMMANDS") assert.NotContains(t, out, "CORE COMMANDS") - // Should contain the subcommand's own description - assert.Contains(t, out, "todos") +} + +func TestCommandHelpRendersExample(t *testing.T) { + isolateHelpTest(t) + + var buf bytes.Buffer + cmd := NewRootCmd() + cmd.AddCommand(commands.NewProjectsCmd()) + cmd.SetOut(&buf) + cmd.SetArgs([]string{"projects", "--help"}) + _ = cmd.Execute() + + out := buf.String() + assert.Contains(t, out, "EXAMPLES") + assert.Contains(t, out, "basecamp projects list") + assert.Contains(t, out, "INHERITED FLAGS") +} + +func TestLeafCommandHelp(t *testing.T) { + isolateHelpTest(t) + + var buf bytes.Buffer + cmd := NewRootCmd() + cmd.AddCommand(commands.NewProjectsCmd()) + cmd.SetOut(&buf) + cmd.SetArgs([]string{"projects", "list", "--help"}) + _ = cmd.Execute() + + out := buf.String() + assert.Contains(t, out, "USAGE") + assert.Contains(t, out, "FLAGS") + assert.NotContains(t, out, "COMMANDS") + // Inherited flags are curated: salient root flags shown, noise hidden + assert.Contains(t, out, "--json") + assert.Contains(t, out, "--quiet") + assert.NotContains(t, out, "--verbose") + assert.NotContains(t, out, "--styled") + // Leaf LEARN MORE points back to parent + assert.Contains(t, out, "basecamp projects --help") +} + +func TestGroupCommandShowsPersistentLocalFlags(t *testing.T) { + // Commands that define their own persistent flags (--project, --in, etc.) + // must show them in FLAGS. This catches regressions where LocalFlags() is + // accidentally replaced with LocalNonPersistentFlags(). + isolateHelpTest(t) + + tests := []struct { + name string + command string + addCmd func() *cobra.Command + wantFlag string + }{ + {"messages --project", "messages", commands.NewMessagesCmd, "--project"}, + {"messages --in", "messages", commands.NewMessagesCmd, "--in"}, + {"messages --message-board", "messages", commands.NewMessagesCmd, "--message-board"}, + {"campfire --project", "campfire", commands.NewCampfireCmd, "--project"}, + {"campfire --campfire", "campfire", commands.NewCampfireCmd, "--campfire"}, + {"campfire --content-type", "campfire", commands.NewCampfireCmd, "--content-type"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + cmd := NewRootCmd() + cmd.AddCommand(tt.addCmd()) + cmd.SetOut(&buf) + cmd.SetArgs([]string{tt.command, "--help"}) + _ = cmd.Execute() + + assert.Contains(t, buf.String(), tt.wantFlag) + }) + } +} + +func TestRootLevelLeafCommandHelp(t *testing.T) { + // Root-level leaf commands (no subcommands, parent is root) must still + // render a complete LEARN MORE section pointing to the root. + isolateHelpTest(t) + + var buf bytes.Buffer + cmd := NewRootCmd() + cmd.AddCommand(commands.NewDoneCmd()) + cmd.SetOut(&buf) + cmd.SetArgs([]string{"done", "--help"}) + _ = cmd.Execute() + + out := buf.String() + assert.Contains(t, out, "USAGE") + assert.Contains(t, out, "INHERITED FLAGS") + assert.Contains(t, out, "LEARN MORE") + assert.Contains(t, out, "basecamp --help") + assert.NotContains(t, out, "COMMANDS") +} + +func TestLeafCommandInheritsParentPersistentFlags(t *testing.T) { + // Leaf commands must show parent-defined persistent flags in INHERITED + // FLAGS. These flags carry required context (--project, --campfire, etc.) + // and hiding them breaks discoverability. + isolateHelpTest(t) + + tests := []struct { + name string + args []string + addCmd func() *cobra.Command + wantFlags []string + }{ + { + "messages create inherits --project", + []string{"messages", "create", "--help"}, + commands.NewMessagesCmd, + []string{"--project", "--in", "--message-board"}, + }, + { + "campfire post inherits --project and --campfire", + []string{"campfire", "post", "--help"}, + commands.NewCampfireCmd, + []string{"--project", "--campfire", "--content-type"}, + }, + { + "timesheet report inherits date and person flags", + []string{"timesheet", "report", "--help"}, + commands.NewTimesheetCmd, + []string{"--project", "--start", "--end", "--person"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + cmd := NewRootCmd() + cmd.AddCommand(tt.addCmd()) + cmd.SetOut(&buf) + cmd.SetArgs(tt.args) + _ = cmd.Execute() + + out := buf.String() + for _, flag := range tt.wantFlags { + assert.Contains(t, out, flag) + } + }) + } } func TestAgentHelpProducesJSON(t *testing.T) { diff --git a/internal/cli/root.go b/internal/cli/root.go index 85a388c8..a2480369 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -179,9 +179,9 @@ func NewRootCmd() *cobra.Command { _ = cmd.RegisterFlagCompletionFunc("account", completer.AccountCompletion()) _ = cmd.RegisterFlagCompletionFunc("profile", completer.ProfileCompletion()) - // Custom root help: gh-style curated categories for root, agent JSON for - // --agent, default cobra help for subcommands. - cmd.SetHelpFunc(rootHelpFunc(cmd.HelpFunc())) + // Styled help: curated categories for root, agent JSON for --agent, + // renderCommandHelp for all subcommands. + cmd.SetHelpFunc(rootHelpFunc()) // Compact usage for the root command only — prevents cobra from dumping // all 55 commands on error. Subcommands inherit cobra's default. diff --git a/internal/commands/commands.go b/internal/commands/commands.go index dd0274ae..fcf16e57 100644 --- a/internal/commands/commands.go +++ b/internal/commands/commands.go @@ -158,24 +158,6 @@ func CatalogCommandNames() []string { return names } -// HelpFunc is a function that renders help for a command. -type HelpFunc func(cmd *cobra.Command, args []string) - -var customHelp = map[*cobra.Command]HelpFunc{} - -// RegisterCustomHelp registers a custom help renderer for a command. -// rootHelpFunc dispatches to these before falling through to cobra's default, -// so --agent mode and child commands are handled correctly. -func RegisterCustomHelp(cmd *cobra.Command, fn HelpFunc) { - customHelp[cmd] = fn -} - -// CustomHelp returns the registered custom help function for a command, if any. -func CustomHelp(cmd *cobra.Command) (HelpFunc, bool) { - fn, ok := customHelp[cmd] - return fn, ok -} - // NewCommandsCmd creates the commands listing command. func NewCommandsCmd() *cobra.Command { return &cobra.Command{ diff --git a/internal/commands/projects.go b/internal/commands/projects.go index d2c100ac..160ab27b 100644 --- a/internal/commands/projects.go +++ b/internal/commands/projects.go @@ -8,7 +8,6 @@ import ( "strings" "github.com/spf13/cobra" - "github.com/spf13/pflag" "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" @@ -25,13 +24,12 @@ func NewProjectsCmd() *cobra.Command { Short: "Manage projects", Long: "Manage Basecamp projects.", Annotations: map[string]string{"agent_notes": "Project IDs appear in Basecamp URLs as the buckets segment: /buckets//...\nbasecamp config project sets the default project for the current repo\nCreating a project returns its ID — use it with basecamp config set project_id "}, - RunE: func(cmd *cobra.Command, args []string) error { - return cmd.Help() - }, + Example: ` $ basecamp projects list + $ basecamp projects list --status archived + $ basecamp projects show 12345 + $ basecamp projects create --name "New project"`, } - RegisterCustomHelp(cmd, renderProjectsHelp) - cmd.AddCommand( newProjectsListCmd(), newProjectsShowCmd(), @@ -43,148 +41,6 @@ func NewProjectsCmd() *cobra.Command { return cmd } -func renderProjectsHelp(cmd *cobra.Command, _ []string) { - r := output.NewRenderer(cmd.OutOrStdout(), false) - var b strings.Builder - - // Subcommand help - if cmd.HasParent() && cmd.Parent().Name() == "projects" { - renderSubcommandHelp(&b, r, cmd) - fmt.Fprint(cmd.OutOrStdout(), b.String()) - return - } - - // Group help - b.WriteString("Manage Basecamp projects.\n") - - b.WriteString("\n") - b.WriteString(r.Header.Render("USAGE")) - b.WriteString("\n") - b.WriteString(" basecamp projects [flags]\n") - - b.WriteString("\n") - b.WriteString(r.Header.Render("COMMANDS")) - b.WriteString("\n") - - type entry struct{ name, desc string } - entries := []entry{ - {"list", "List projects"}, - {"show", "Show project details"}, - {"create", "Create a new project"}, - {"update", "Update a project"}, - {"delete", "Delete (trash) a project"}, - } - for _, e := range entries { - fmt.Fprintf(&b, " %-10s %s\n", e.name, e.desc) - } - - b.WriteString("\n") - b.WriteString(r.Header.Render("FLAGS")) - b.WriteString("\n") - b.WriteString(" -p, --project Project ID or name\n") - b.WriteString(" --help Show help for command\n") - - b.WriteString("\n") - b.WriteString(r.Header.Render("EXAMPLES")) - b.WriteString("\n") - examples := []string{ - "$ basecamp projects list", - "$ basecamp projects list --status archived", - "$ basecamp projects show 12345", - `$ basecamp projects create --name "New project"`, - } - for _, ex := range examples { - b.WriteString(r.Muted.Render(" "+ex) + "\n") - } - - b.WriteString("\n") - b.WriteString(r.Header.Render("LEARN MORE")) - b.WriteString("\n") - b.WriteString(" basecamp projects -h Help for any subcommand\n") - - fmt.Fprint(cmd.OutOrStdout(), b.String()) -} - -// subcommandExamples maps subcommand names to their example lines. -var subcommandExamples = map[string][]string{ - "list": { - "$ basecamp projects list", - "$ basecamp projects list --status archived", - "$ basecamp projects list --limit 10", - }, - "show": { - "$ basecamp projects show 12345", - "$ basecamp projects show 12345 --json", - }, - "create": { - `$ basecamp projects create --name "Q1 Planning"`, - `$ basecamp projects create --name "Redesign" --description "Website redesign"`, - }, - "update": { - `$ basecamp projects update 12345 --name "New name"`, - `$ basecamp projects update 12345 --description "Updated description"`, - }, - "delete": { - "$ basecamp projects delete 12345", - }, -} - -func renderSubcommandHelp(b *strings.Builder, r *output.Renderer, cmd *cobra.Command) { - b.WriteString(cmd.Long) - b.WriteString("\n") - - b.WriteString("\n") - b.WriteString(r.Header.Render("USAGE")) - b.WriteString("\n") - fmt.Fprintf(b, " basecamp projects %s\n", cmd.Use) - - // Local flags (command-specific) - if cmd.HasLocalFlags() { - hasLocal := false - var flags strings.Builder - cmd.LocalFlags().VisitAll(func(f *pflag.Flag) { - if f.Hidden || f.Name == "help" { - return - } - hasLocal = true - if f.Shorthand != "" { - fmt.Fprintf(&flags, " -%s, --%-14s %s\n", f.Shorthand, f.Name, f.Usage) - } else { - fmt.Fprintf(&flags, " --%-14s %s\n", f.Name, f.Usage) - } - }) - if hasLocal { - b.WriteString("\n") - b.WriteString(r.Header.Render("FLAGS")) - b.WriteString("\n") - b.WriteString(flags.String()) - } - } - - // Curated inherited flags - b.WriteString("\n") - b.WriteString(r.Header.Render("INHERITED FLAGS")) - b.WriteString("\n") - b.WriteString(" --help Show help for command\n") - b.WriteString(" -j, --json Output as JSON\n") - b.WriteString(" -q, --quiet Quiet output\n") - - // Examples - if examples, ok := subcommandExamples[cmd.Name()]; ok { - b.WriteString("\n") - b.WriteString(r.Header.Render("EXAMPLES")) - b.WriteString("\n") - for _, ex := range examples { - b.WriteString(r.Muted.Render(" "+ex) + "\n") - } - } - - b.WriteString("\n") - b.WriteString(r.Header.Render("LEARN MORE")) - b.WriteString("\n") - b.WriteString(" basecamp projects -h Show all projects commands\n") -} - func newProjectsListCmd() *cobra.Command { var status string var limit, page int