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

improv: Use anyhow::Error instead of String for error types #185

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Aug 25, 2020

This is generally a better practice. It seems to make the code slightly neater, or comparable.

BREAKING CHANGE: This changes the public API of the crate. However, it shouldn't change the behavior of the program other than somewhat different error messages for crashes.

This is generally a better practice. It seems to make the code slightly
neater, or comparable.

BREAKING CHANGE: This changes the public API of the crate. However, it
shouldn't change the behavior of the program other than somewhat
different error messages for crashes.
@ids1024 ids1024 requested review from a team August 25, 2020 15:41
@jacobgkau
Copy link
Member

The error messages for an unexpected argument and for the daemon needing to be run as root look identical before and after the change. The error message for if the daemon's not running is less descriptive after the change. Before:

daemon returned an error message: "The name com.system76.PowerDaemon was not provided by any .service files"

After:

daemon returned an error message

The former message gives a bigger hint to check if the system76-power service is running. Is removing that part necessary/intentional?

@ids1024
Copy link
Member Author

ids1024 commented Sep 3, 2020

Hm. No, not intentional. That's definitely not a descriptive enough message.

@ids1024 ids1024 marked this pull request as draft September 3, 2020 21:19
Copy link

@bflanagin bflanagin left a comment

Choose a reason for hiding this comment

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

This PR seems to be out of date, and needs attention. Will reassess when updated.

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