-
Notifications
You must be signed in to change notification settings - Fork 257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Colorify errors #1497
Colorify errors #1497
Conversation
src/commands/external.rs
Outdated
@@ -50,7 +51,7 @@ pub async fn execute_external_subcommand( | |||
} | |||
Err(Error::NotFound(e)) => { | |||
tracing::debug!("Tried to resolve {plugin_name} to plugin, got {e}"); | |||
eprintln!("Error: '{plugin_name}' is not a known Spin command. See spin --help.\n"); | |||
error!("'{plugin_name}' is not a known Spin command. See spin --help.\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if print_error!
might make it clearer that this is a print operation. This looks too much like bail!
for me to readily infer that this is a benign piece of user interaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as tracing::error
and log::error
. How about having the convention of always using this in a qualified way colors::error!
is hopefully less prone to confusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be better though in that case I'd suggest a namespace like console
or terminal
- tracing
and log
directly imply output in a way that colors
(to me) doesn't.
Which might be better because the formatting conventions could come to involve icon-based highlighting or layout functions as well, although I realise this is speculative!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed the dependency to terminal
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy with this (whether or not you decide to change the namespace, and we can always iterate on that). Thanks!
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
This is the first step for #1481. Errors are now proceeded by "Error" in bold red text. This should happen for all errors and the user's browser settings such as
NO_COLOR
are respected.