added subgroup name #285
Merged
added subgroup name #285
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR introduces configurable command grouping in the root help output by adding a Sequence diagram for grouped root help renderingsequenceDiagram
actor User
participant Main
participant RootFlags
participant RootCommand
participant RootHelp as PrintRootHelpMessage
participant Cobra as CobraFramework
User->>Main: invoke_lets_with_help_flag
Main->>RootFlags: parse_flags
RootFlags-->>Main: help_true
Main->>RootCommand: initialize_root_command
Main->>RootHelp: PrintRootHelpMessage(rootCmd)
activate RootHelp
RootHelp->>RootCommand: Short
RootHelp->>RootCommand: UseLine()
RootHelp->>RootCommand: CommandPath()
loop for_each_group
RootHelp->>RootCommand: Groups()
RootHelp->>RootHelp: buildGroupCommandHelp(rootCmd, group)
activate RootHelp
RootHelp->>RootCommand: Commands()
RootHelp->>RootCommand: NamePadding()
RootHelp-->>RootHelp: group_and_subgroup_output
deactivate RootHelp
end
RootHelp->>RootCommand: HasAvailableLocalFlags()
alt has_flags
RootHelp->>RootCommand: LocalFlags().FlagUsagesWrapped(120)
end
RootHelp->>RootCommand: CommandPath()
RootHelp->>RootCommand: OutOrStdout()
RootHelp-->>User: grouped_help_text
deactivate RootHelp
Class diagram for updated command configuration and help wiringclassDiagram
class Command {
+string Name
+string GroupName
+Cmds Cmds
+UnmarshalYAML(unmarshal)
+Clone()
+GetEnv(cfg)
}
class CobraCommand {
+string UseLine()
+string CommandPath()
+string Short
+string Version
+string Name()
+string GroupID
+string Title
+map_string_string Annotations
+bool Runnable()
+bool HasAvailableSubCommands()
+bool HasAvailableLocalFlags()
+CobraCommand_array Commands()
+CobraGroup_array Groups()
+int NamePadding()
+Flags LocalFlags()
+Writer OutOrStdout()
}
class CobraGroup {
+string ID
+string Title
}
class RootHelp {
+buildGroupCommandHelp(cmd, group) string
+PrintRootHelpMessage(cmd) error
}
class SubcommandFactory {
+newSubcommand(command, conf, showAll, out) CobraCommand
}
Command --> SubcommandFactory : input_to
SubcommandFactory --> CobraCommand : create_subcommand
CobraCommand --> RootHelp : passed_into
RootHelp --> CobraGroup : iterate_groups
CobraCommand --> CobraGroup : grouped_by_ID
Command --> Command : Clone
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Arfey
commented
Jan 7, 2026
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new
buildGroupedCommandHelpfunction iterates over maps (groupCmdMapand the innerGroupsMap) which leads to non-deterministic ordering; if the help output or tests depend on a stable order, consider sorting groups/subgroups/commands instead of ranging over maps directly. - In
PrintRootHelpMessage, you no longer prepend a"Commands:\n\n"section header before the grouped command list, but the new Bats fixture expects that header, so either reintroduce it or update the expected help text for consistency. - The
UnmarshalYAMLdefaulting logic setsGroupNameto "Common" whengroup_nameis omitted, butTestParseCommandcurrently expects an empty string for the default; align the test expectation or the defaulting behavior so they match.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `buildGroupedCommandHelp` function iterates over maps (`groupCmdMap` and the inner `GroupsMap`) which leads to non-deterministic ordering; if the help output or tests depend on a stable order, consider sorting groups/subgroups/commands instead of ranging over maps directly.
- In `PrintRootHelpMessage`, you no longer prepend a `"Commands:\n\n"` section header before the grouped command list, but the new Bats fixture expects that header, so either reintroduce it or update the expected help text for consistency.
- The `UnmarshalYAML` defaulting logic sets `GroupName` to "Common" when `group_name` is omitted, but `TestParseCommand` currently expects an empty string for the default; align the test expectation or the defaulting behavior so they match.
## Individual Comments
### Comment 1
<location> `cmd/root.go:61-68` </location>
<code_context>
return err
}
+func buildGroupedCommandHelp(cmd *cobra.Command) string {
+ help := ""
+ cmds := cmd.Commands()
+ groups := cmd.Groups()
+
+ groupCmdMap := make(map[string]map[string][]*cobra.Command)
+
+ // todo: add sort
+
+ for _, group := range groups {
</code_context>
<issue_to_address>
**issue (bug_risk):** Iteration over maps will produce non-deterministic group and command ordering.
Because both `groupCmdMap` and its nested maps are iterated with `for ... range`, the help output order will vary between runs, which can cause flaky tests and noisy doc diffs. Since there’s already a TODO for sorting, consider using the existing `groups := cmd.Groups()` slice and sorting commands within each group (e.g., by `Use` or `Name()`) to ensure stable output.
</issue_to_address>
### Comment 2
<location> `cmd/subcommand.go:205-206` </location>
<code_context>
c.Println(err)
}
})
+ subCmd.Annotations = map[string]string{
+ "SubGroupName": command.GroupName,
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Assigning a new Annotations map may clobber existing annotations on the command.
This overwrites any existing `subCmd.Annotations` set elsewhere (now or in future), which can lead to unexpected behavior. Instead, only initialize when nil and then set the key, e.g.:
```go
if subCmd.Annotations == nil {
subCmd.Annotations = map[string]string{}
}
subCmd.Annotations["SubGroupName"] = command.GroupName
```
</issue_to_address>
### Comment 3
<location> `config/config/command_test.go:22-29` </location>
<code_context>
+}
+
+func TestParseCommand(t *testing.T) {
+ t.Run("default group_name", func(t *testing.T) {
+ text := dedent.Dedent(`
+ cmd: [echo, Hello]
+ `)
+ command := CommandFixture(t, text)
+ exp := ""
+
+ if command.GroupName != exp {
+ t.Errorf("wrong output. \nexpect %s \ngot: %s", exp, command.GroupName)
+ }
</code_context>
<issue_to_address>
**issue (testing):** The "default group_name" test expectation contradicts the implementation default of "Common" and should be updated.
In `UnmarshalYAML`, `GroupName` defaults to `"Common"` when `group_name` is omitted, but this test expects an empty string. Please update the expected value to `"Common"` (and the assertion message if needed) so the test matches the actual default behavior.
</issue_to_address>
### Comment 4
<location> `tests/command_group_name.bats:9-18` </location>
<code_context>
+HELP_MESSAGE=$(cat <<EOF
</code_context>
<issue_to_address>
**issue (testing):** The help output assertion is brittle because the implementation uses Go maps without sorting, so order is not guaranteed.
The expected `HELP_MESSAGE` depends on a fixed ordering of groups and commands, but `buildGroupedCommandHelp` uses nested Go maps (`map[string]map[string][]*cobra.Command`) without sorting (see `// todo: add sort`), so output order is randomized and the test becomes flaky.
To fix this, either:
- Sort group names, subgroup names, and commands before formatting so a full-string assertion is valid, or
- Adjust the test to assert on key substrings/structure (e.g., via `assert_line` for each group/command and indentation) without relying on relative order.
The test should avoid depending on Go map iteration order.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+205
to
+206
| subCmd.Annotations = map[string]string{ | ||
| "SubGroupName": command.GroupName, |
There was a problem hiding this comment.
issue (bug_risk): Assigning a new Annotations map may clobber existing annotations on the command.
This overwrites any existing subCmd.Annotations set elsewhere (now or in future), which can lead to unexpected behavior. Instead, only initialize when nil and then set the key, e.g.:
if subCmd.Annotations == nil {
subCmd.Annotations = map[string]string{}
}
subCmd.Annotations["SubGroupName"] = command.GroupName
kindermax
reviewed
Jan 7, 2026
kindermax
reviewed
Jan 7, 2026
| } | ||
|
|
||
|
|
||
| func PrintRootHelpMessage(cmd *cobra.Command) error { |
Collaborator
There was a problem hiding this comment.
Looks like if lets executed as is (without help command) there will be no groups in the output
kindermax
reviewed
Jan 7, 2026
Collaborator
|
@Arfey overall looks good, lets fix remaining issues and its ready for merge |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The hard-coded strings like
"Common"for the default group name and"SubGroupName"for the annotation key are used in multiple places; consider extracting them into shared constants to avoid typos and make future changes safer. - In
buildGroupCommandHelp, commands without aSubGroupNameare handled in a separate loop after subgroup rendering; you may want to guard against accidentally printing a command twice if annotations are manipulated elsewhere by tightening the selection logic or documenting the assumption. - The naming of the
SubGroupNameannotation is a bit confusing given the YAML key isgroupand the docs talk about groups; consider aligning this terminology (e.g.,GroupNameeverywhere) to make it clearer how grouping is configured and rendered.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hard-coded strings like `"Common"` for the default group name and `"SubGroupName"` for the annotation key are used in multiple places; consider extracting them into shared constants to avoid typos and make future changes safer.
- In `buildGroupCommandHelp`, commands without a `SubGroupName` are handled in a separate loop after subgroup rendering; you may want to guard against accidentally printing a command twice if annotations are manipulated elsewhere by tightening the selection logic or documenting the assumption.
- The naming of the `SubGroupName` annotation is a bit confusing given the YAML key is `group` and the docs talk about groups; consider aligning this terminology (e.g., `GroupName` everywhere) to make it clearer how grouping is configured and rendered.
## Individual Comments
### Comment 1
<location> `cmd/subcommand.go:205-207` </location>
<code_context>
c.Println(err)
}
})
+ subCmd.Annotations = map[string]string{
+ "SubGroupName": command.GroupName,
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Overwriting `subCmd.Annotations` may discard existing annotations.
If `subCmd` may already have annotations (now or later), this assignment will discard them. Instead, ensure `subCmd.Annotations` is initialized if nil, then set `subCmd.Annotations["SubGroupName"] = command.GroupName` so existing entries are preserved.
```suggestion
if subCmd.Annotations == nil {
subCmd.Annotations = make(map[string]string)
}
subCmd.Annotations["SubGroupName"] = command.GroupName
```
</issue_to_address>
### Comment 2
<location> `config/config/command_test.go:5` </location>
<code_context>
+
+import (
+ "bytes"
+ "testing"
+
+ "github.com/lithammer/dedent"
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case for an explicitly empty `group:` field defaulting to "Common"
The unmarshal logic treats `group: ""` the same as a missing `group` (both default to "Common"). Existing tests cover the missing-group case and a custom group, but not the explicitly empty string. Please add a subtest for `group: ""` to capture this behavior and prevent regressions in the defaulting logic.
Suggested implementation:
```golang
func CommandFixture(t *testing.T, text string) *Command {
buf := bytes.NewBufferString(text)
c := &Command{}
if err := yaml.NewDecoder(buf).Decode(&c); err != nil {
t.Fatalf("command fixture decode error: %s", err)
}
return c
}
func TestCommandUnmarshal_EmptyGroupDefaultsToCommon(t *testing.T) {
yamlText := dedent.Dedent(`
name: example
command: echo "hello"
group: ""
`)
cmd := CommandFixture(t, yamlText)
if cmd.Group != "Common" {
t.Fatalf("expected Group to default to %q for empty group field, got %q", "Common", cmd.Group)
}
}
```
If `CommandFixture` already returns `c` in the current file (the snippet may be truncated), you should:
1. Remove the duplicate `return c` introduced by this edit and keep only a single `return c` before the helper’s closing brace.
2. Optionally, if you want this to be a subtest under an existing test (e.g., `TestCommandUnmarshal`), you can move the body of `TestCommandUnmarshal_EmptyGroupDefaultsToCommon` into a `t.Run("empty group defaults to Common", func(t *testing.T) { ... })` inside that existing test instead of having a standalone test function.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+205
to
+207
| subCmd.Annotations = map[string]string{ | ||
| "SubGroupName": command.GroupName, | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Overwriting subCmd.Annotations may discard existing annotations.
If subCmd may already have annotations (now or later), this assignment will discard them. Instead, ensure subCmd.Annotations is initialized if nil, then set subCmd.Annotations["SubGroupName"] = command.GroupName so existing entries are preserved.
Suggested change
| subCmd.Annotations = map[string]string{ | |
| "SubGroupName": command.GroupName, | |
| } | |
| if subCmd.Annotations == nil { | |
| subCmd.Annotations = make(map[string]string) | |
| } | |
| subCmd.Annotations["SubGroupName"] = command.GroupName |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#60
Summary by Sourcery
Group CLI subcommands in help output by configurable group names and wire command configuration through to the root help display.
New Features:
Tests:
lets --helpshows commands grouped under their configured group names.Example
Summary by Sourcery
Group CLI commands in the root help output by configurable group names derived from command configuration.
New Features:
Documentation:
Tests: