cmd/cli/commands: use local fork of hooks.PrintNextSteps#763
cmd/cli/commands: use local fork of hooks.PrintNextSteps#763ilopezluna merged 1 commit intodocker:mainfrom
Conversation
We're refactoring code in the CLI for better separation of plugin-manager and plugin code; add a local fork of this utility instead of depending on the utility in the CLI. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
![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 left some high level feedback:
- Now that
printNextSteps/boldare locally defined, consider centralizing ANSI styling helpers (or at least adding a brief comment/TODO) so future changes to how next-step messages are formatted don’t silently diverge from other CLI output conventions. - If
printNextStepsoutput may be consumed in non-TTY contexts (e.g., piping to files/tools), you might want to guard the ANSI escape codes or offer a way to disable them to avoid emitting raw control sequences.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `printNextSteps`/`bold` are locally defined, consider centralizing ANSI styling helpers (or at least adding a brief comment/TODO) so future changes to how next-step messages are formatted don’t silently diverge from other CLI output conventions.
- If `printNextSteps` output may be consumed in non-TTY contexts (e.g., piping to files/tools), you might want to guard the ANSI escape codes or offer a way to disable them to avoid emitting raw control sequences.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 refactors the code to use a local implementation of PrintNextSteps, removing the dependency on github.com/docker/cli/cli-plugins/hooks. The change is straightforward and achieves its goal. My review includes a suggestion to improve the new printNextSteps function to handle terminal styling correctly, preventing raw ANSI escape codes from appearing in non-terminal outputs like error messages.
| func printNextSteps(out io.Writer, messages []string) { | ||
| if len(messages) == 0 { | ||
| return | ||
| } | ||
| _, _ = fmt.Fprintln(out, bold("\nWhat's next:")) | ||
| for _, n := range messages { | ||
| _, _ = fmt.Fprintln(out, " ", n) | ||
| } | ||
| } |
There was a problem hiding this comment.
The bold function unconditionally adds ANSI escape codes. This is problematic when printNextSteps is called with a non-terminal writer, such as a buffer for an error message in handleClientError. The resulting error string would contain raw escape codes, which can interfere with logging and error parsing.
To address this, printNextSteps should detect if its output writer is a terminal and only apply bold styling when it is. This ensures clean output for both terminal and non-terminal destinations.
| func printNextSteps(out io.Writer, messages []string) { | |
| if len(messages) == 0 { | |
| return | |
| } | |
| _, _ = fmt.Fprintln(out, bold("\nWhat's next:")) | |
| for _, n := range messages { | |
| _, _ = fmt.Fprintln(out, " ", n) | |
| } | |
| } | |
| func printNextSteps(out io.Writer, messages []string) { | |
| if len(messages) == 0 { | |
| return | |
| } | |
| isTerminal := false | |
| if f, ok := out.(*os.File); ok { | |
| _, isTerminal = term.GetFdInfo(f) | |
| } | |
| header := "\nWhat's next:" | |
| if isTerminal { | |
| header = bold(header) | |
| } | |
| _, _ = fmt.Fprintln(out, header) | |
| for _, n := range messages { | |
| _, _ = fmt.Fprintln(out, " ", n) | |
| } | |
| } |
There was a problem hiding this comment.
This is a straight replacement of the existing implementation (but cutting the aec dependency out as intermediate).
Having it as a local implementation would also for enhancements like this, but currently it's just a swap of the code.
2 participants
We're refactoring code in the CLI for better separation of plugin-manager and plugin code; add a local fork of this utility instead of depending on the utility in the CLI.