Skip to content
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

add better error reporting #789

Merged
merged 3 commits into from
Jan 21, 2021
Merged

Conversation

robinvd
Copy link
Contributor

@robinvd robinvd commented Jan 21, 2021

use color_eyre to display errors, instead of just panicing and hoping the user will read through the long error to find whats wrong
this initial commit only has better errors for config loading as that is propably the most common error

for example:

Loading config from "/home/robin.local/.config/spotifyd/spotifyd.conf"
Error: 
   0: could not load the config file
   1: invalid type: integer `90`, expected a string for key `global.initial_volume` at line 8 column 18

Location:
   src/config.rs:478

note:
   the config format should be valid TOML
   we recently changed the config format, check the README for more info

Backtrace omitted.
Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.

use color_eyre to display errors, instead of just panicing and hoping the user will read through the long error to find whats wrong

this initial commit only has better errors for config loading as that is propably the most common error
src/main.rs Outdated
.with_section(|| {
concat!(
"the config format should be valid TOML\n",
"we recently changed the config format, check the README for more info"
Copy link
Member

@JojiiOfficial JojiiOfficial Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine! The only change I'd come up with would be to add a link to the certain github issue(s) here. That would make it easier to troubleshoot the known config issue(s). What do you think?

Edit: Maybe we should remove/improve line 67 because it looks like two errors appeared:

Error:
   0: could not load the config file
   1: invalid type: integer `90`, expected a string for key `global.initial_volume` at line 69 column 18

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the way color-eyre provides a stack of context, from the actual error to what that error means in the application. I can see how it looks like 2 errors, ill look if the lib supports some customization otherwise maybe we should remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm i cant seem to change the Error: to something like Caused by:, but removing the line could not load the config file would make it less obvious that the error came from loading the config file. So i think this is the best for now

Copy link
Contributor Author

@robinvd robinvd Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only change I'd come up with would be to add a link to the certain github issue(s) here

there is even some support for generating github issues (https://docs.rs/color-eyre/0.5.10/color_eyre/config/struct.HookBuilder.html#method.issue_url).

@mainrs
Copy link
Member

mainrs commented Jan 21, 2021

It's even tracing-compatible, which makes it even great! The error message does look pretty good.

@robinvd
Copy link
Contributor Author

robinvd commented Jan 21, 2021

after adding github url:

Loading config from "/home/robin.local/.config/spotifyd/spotifyd.conf"
Error: 
   0: could not load the config file
   1: invalid number at line 2 column 15

Location:
   src/config.rs:478

note:
   the config format should be valid TOML
   we recently changed the config format, see https://github.com/Spotifyd/spotifyd/issues/765

Backtrace omitted.
Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.

@JojiiOfficial JojiiOfficial merged commit 25d0afe into Spotifyd:master Jan 21, 2021
@robinvd
Copy link
Contributor Author

robinvd commented Jan 21, 2021

A quick patch release might be a good idea, as most users might benefit from this improved error

@JojiiOfficial
Copy link
Member

Sounds good!

@eladyn eladyn mentioned this pull request Aug 26, 2022
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