Conversation
📝 WalkthroughWalkthroughA new global help command was added and registered with the dispatcher via 📝 Coding Plan
Comment |
phinze
left a comment
There was a problem hiding this comment.
LGTM - should this get represented in the docs somehow? Maybe just as an autogen'd CLI reference page for help?
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cli/commands/commands.go (1)
845-846: Comment says “registered last,” but that is not true after Line [848].Please either move help registration below
addCommands(d)or update the comment to avoid misleading future readers.🛠️ Minimal comment-only fix
- // Help command (registered last so it can reference all other commands) + // Help command (uses dispatcher state at runtime to show help for registered commands) d.Dispatch("help", NewHelpCommand(d))As per coding guidelines: "Only add comments when they provide valuable context or explain 'why' something is done - Avoid redundant comments that restate what the code does".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/commands/commands.go` around lines 845 - 846, The comment claiming the help command is "registered last" is inaccurate because addCommands(d) runs afterwards; either move the help registration call Dispatch("help", NewHelpCommand(d)) to after addCommands(d) so it truly is registered last, or update the comment to remove/mcorrect the "registered last" phrasing (e.g., explain intended ordering or that help is registered among other commands) so readers aren't misled.cli/commands/help_commands.go (1)
44-55: Validate--formatand reject conflicting help modes explicitly.Line [45] currently ignores positional command names when
--commandsis set, and Lines [66]/[96] silently treat unknown formats as table/text. Returning explicit errors here will make CLI behavior predictable.💡 Proposed refactor
func (h *helpCommand) Run(fs *mflags.FlagSet, args []string) error { + format := strings.ToLower(strings.TrimSpace(h.format)) + switch format { + case "table", "json": + h.format = format + default: + return fmt.Errorf("invalid --format %q (expected: table or json)", h.format) + } + + if h.listCommands && len(h.commands) > 0 { + return fmt.Errorf("--commands cannot be combined with command names") + } + if h.listCommands { return h.runListCommands() }Also applies to: 63-71, 95-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/commands/help_commands.go` around lines 44 - 55, The Run method on helpCommand currently allows conflicting modes and silently accepts unknown --format values; update helpCommand.Run to explicitly validate conflicts and formats: if h.listCommands && len(h.commands) > 0 return an error indicating --commands conflicts with positional command names, and validate h.format against the allowed set (e.g. "table","text","json") before dispatching to runListCommands or runMultiHelp; on unknown format return an explicit error instead of falling back. Ensure the checks occur at the top of helpCommand.Run (before calling runListCommands or runMultiHelp) and include clear error messages referencing the --format flag and the conflicting modes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cli/commands/commands.go`:
- Around line 845-846: The comment claiming the help command is "registered
last" is inaccurate because addCommands(d) runs afterwards; either move the help
registration call Dispatch("help", NewHelpCommand(d)) to after addCommands(d) so
it truly is registered last, or update the comment to remove/mcorrect the
"registered last" phrasing (e.g., explain intended ordering or that help is
registered among other commands) so readers aren't misled.
In `@cli/commands/help_commands.go`:
- Around line 44-55: The Run method on helpCommand currently allows conflicting
modes and silently accepts unknown --format values; update helpCommand.Run to
explicitly validate conflicts and formats: if h.listCommands && len(h.commands)
> 0 return an error indicating --commands conflicts with positional command
names, and validate h.format against the allowed set (e.g.
"table","text","json") before dispatching to runListCommands or runMultiHelp; on
unknown format return an explicit error instead of falling back. Ensure the
checks occur at the top of helpCommand.Run (before calling runListCommands or
runMultiHelp) and include clear error messages referencing the --format flag and
the conflicting modes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 511c22f6-d203-4149-938e-4342bb26b890
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
cli/commands/commands.gocli/commands/help_commands.gogo.mod
Add `miren help --commands` to list all commands with synopsis (supports --format json) and `miren help cmd1 cmd2 ...` to show help for multiple commands at once using dot notation (e.g. app.list → app list). The help command delegates to the dispatcher's ShowCommandHelp so output matches `miren <cmd> -h` exactly. Also updates mflags to export ShowCommandHelp and to let a registered "help" command handle its own execution instead of being intercepted as a help keyword.
3f5257a to
8c794aa
Compare
|
@phinze it will be include on all miren.md automatically if that's what you mean? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/commands/help_commands.go (1)
90-102: Extract command-path normalization to a helper to avoid drift.Line 90 and Line 101 duplicate dot-notation normalization. A tiny helper keeps JSON/text paths consistent if behavior changes later.
Refactor sketch
+func normalizeHelpPath(name string) string { + return strings.ReplaceAll(name, ".", " ") +} + func runMultiHelp(d *mflags.Dispatcher, opts helpOpts) error { @@ - path := strings.ReplaceAll(name, ".", " ") + path := normalizeHelpPath(name) @@ - path := strings.ReplaceAll(name, ".", " ") + path := normalizeHelpPath(name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/commands/help_commands.go` around lines 90 - 102, The code duplicates dot-to-space normalization (strings.ReplaceAll(name, ".", " ")) in help_commands.go; extract this into a single helper (e.g., normalizeCommandPath(name string) string or commandPathFromName) and replace both occurrences where path is assigned (the two places using strings.ReplaceAll) with calls to that helper so command-path normalization is centralized and consistent for both JSON and text paths (leave PrintJSON/results logic unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/commands/help_commands.go`:
- Around line 28-33: The current logic lets opts.ListCommands silently override
positional command names (opts.Commands) by calling runListCommands when both
are present; add an explicit conflict check before the existing branches: if
opts.ListCommands && len(opts.Commands) > 0 then return a usage/error indicating
the flags are mutually exclusive (so callers get a clear message) instead of
falling through to runListCommands or runMultiHelp; update the handler that
currently calls runListCommands and runMultiHelp to perform this check and
return an error (or proper usage output) when the conflict is detected.
---
Nitpick comments:
In `@cli/commands/help_commands.go`:
- Around line 90-102: The code duplicates dot-to-space normalization
(strings.ReplaceAll(name, ".", " ")) in help_commands.go; extract this into a
single helper (e.g., normalizeCommandPath(name string) string or
commandPathFromName) and replace both occurrences where path is assigned (the
two places using strings.ReplaceAll) with calls to that helper so command-path
normalization is centralized and consistent for both JSON and text paths (leave
PrintJSON/results logic unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a43817c7-76b6-45e9-844b-7d7a57fd7408
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
cli/commands/commands.gocli/commands/help_commands.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- cli/commands/commands.go
Summary
miren help --commandsto list all commands with their synopsis (supports--format json)miren help app.list version sandbox.stopto show help for multiple commands at once using dot notationShowCommandHelpso it matchesmiren <cmd> -hexactlyShowCommandHelpexport and fix for registered "help" command not being intercepted as a help keywordTest plan
miren help --commandsshows all commands with synopsismiren help --commands --format jsonoutputs JSONmiren help app.list versionshows help for both commandsmiren help app.list version --format jsonoutputs JSON help for bothmiren helpalone still shows the default helpmiren help nonexistentshows a clear errormiren app -hstill works as beforemake lintpasses