Skip to content

Conversation

@Abdorithm
Copy link

Replace fmt.Fprintf(stderr, ...) with slog.Warn(fmt.Sprintf(...)) in the warnIfCommandAmbiguous function and modify tests accordingly. This change:

  • Addresses the existing TODO
  • Makes warning messages consistent with the rest of the codebase
  • Updates snapshots, the warning now appears in stdout instead of stderr

Replace `fmt.Fprintf(stderr, ...)` with `slog.Warn(fmt.Sprintf(...))` in
the `warnIfCommandAmbiguous` function and modify tests accordingly. This
change:
- Addresses the existing TODO
- Makes warning messages consistent with the rest of the codebase
- Updates snapshots, the warning now appears in stdout instead of stderr
@another-rex another-rex requested a review from G-Rath March 20, 2025 23:58
@another-rex
Copy link
Collaborator

I'm not 100% sure we want slog for CLI output, but I suppose using slog right now is better than what we are currently doing. @G-Rath thoughts?

@G-Rath
Copy link
Collaborator

G-Rath commented Mar 21, 2025

the first part of your comment is exactly what I was just typing out - it's why the todo has a "maybe..." in it, though I'm not sure about the "better than what we're currently doing" part.

My take is that this is just an attempt to action the todo, rather than fixing an external issue i.e. is anyone actually complaining that this message is going to stderr instead of stdout? (because no one should be complaining that the output isn't going through slog since this lives in the CLI code which is private, so anyone calling it programmatically from Go where not using slog would matter, is breaking our api contracts anyway)

@another-rex
Copy link
Collaborator

Closing this for now as it seems like we don't want to have this output with slog.

@another-rex another-rex closed this Apr 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants