Conversation
Signed-off-by: samzong <samzong.lu@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a command catalog system for the lathe CLI, adding subcommands to list, show, search, and view the schema of generated commands. The implementation includes a new catalog generation and searching mechanism in the runtime package that leverages cobra command annotations for metadata storage. Review feedback focuses on improving code idiomaticity and performance by replacing the sort package with the slices package for sorting catalog commands, search results, and formatter names.
| import ( | ||
| "encoding/json" | ||
| "slices" | ||
| "sort" |
There was a problem hiding this comment.
The sort package is no longer needed if we switch to the more idiomatic and performant slices.SortFunc for sorting the catalog commands and search results.
| "sort" | |
| "slices" |
| sort.Slice(commands, func(i, j int) bool { | ||
| return slices.Compare(commands[i].Path, commands[j].Path) < 0 | ||
| }) |
There was a problem hiding this comment.
Since Go 1.21, slices.SortFunc is preferred over sort.Slice as it is type-safe and avoids reflection. Additionally, slices.Compare already returns the int value expected by SortFunc, making the implementation cleaner.
| sort.Slice(commands, func(i, j int) bool { | |
| return slices.Compare(commands[i].Path, commands[j].Path) < 0 | |
| }) | |
| slices.SortFunc(commands, func(a, b CatalogCommand) int { | |
| return slices.Compare(a.Path, b.Path) | |
| }) |
| sort.Slice(results, func(i, j int) bool { | ||
| if results[i].Score != results[j].Score { | ||
| return results[i].Score > results[j].Score | ||
| } | ||
| return slices.Compare(results[i].Command.Path, results[j].Command.Path) < 0 | ||
| }) |
There was a problem hiding this comment.
Using slices.SortFunc here improves readability and performance by avoiding reflection. The logic for descending score and ascending path remains clear.
| sort.Slice(results, func(i, j int) bool { | |
| if results[i].Score != results[j].Score { | |
| return results[i].Score > results[j].Score | |
| } | |
| return slices.Compare(results[i].Command.Path, results[j].Command.Path) < 0 | |
| }) | |
| slices.SortFunc(results, func(a, b SearchResult) int { | |
| if a.Score != b.Score { | |
| return b.Score - a.Score | |
| } | |
| return slices.Compare(a.Command.Path, b.Command.Path) | |
| }) |
| import "io" | ||
| import ( | ||
| "io" | ||
| "sort" |
There was a problem hiding this comment.
Prefer using the slices package for sorting simple slices of ordered types like strings.
| "sort" | |
| "slices" |
| } | ||
| extra = append(extra, name) | ||
| } | ||
| sort.Strings(extra) |
There was a problem hiding this comment.
Use slices.Sort for a more modern and efficient way to sort a slice of strings.
| sort.Strings(extra) | |
| slices.Sort(extra) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 737127ab06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cmd.AddCommand(commandsCmd(m)) | ||
| cmd.AddCommand(searchCmd(m)) |
There was a problem hiding this comment.
Adding fixed root commands commands and search without any collision check can break downstream CLIs that already have modules with those names: generated modules are mounted onto root after NewApp, so a module named search/commands will conflict at the same command path and become ambiguous or unreachable. This is a regression for previously valid module names and should be prevented (for example by reserving/validating names at generation time or namespacing these introspection commands).
Useful? React with 👍 / 👎.
| addJSONFlag(cmd, &jsonOut, "Emit catalog JSON") | ||
| cmd.Flags().BoolVar(&includeHidden, "include-hidden", false, "Include hidden commands") |
There was a problem hiding this comment.
commands
The parent commands command defines local --json/--include-hidden flags, and commands show defines flags with the same names again; when users place flags before the subcommand (for example, commands --json show ... or commands --include-hidden show ...), Cobra consumes the parent flag value but show reads its own separate variables, so the request is silently ignored. This makes valid-looking invocations return non-JSON output or fail to find hidden commands unexpectedly.
Useful? React with 👍 / 👎.
1 participant
What's changed?
commands,commands show, andcommands schema.searchfor generated commands.Why