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

Handling unwrap errors in lib.rs #99

Closed
wants to merge 7 commits into from
Closed

Handling unwrap errors in lib.rs #99

wants to merge 7 commits into from

Conversation

yaleman
Copy link

@yaleman yaleman commented Aug 8, 2021

This blind unwrap seems to be a very common error in builds, when it panics it gives the end user no clear way of identifying what's wrong, this PR adds some handling to show the file path that can't be found.

@sfackler
Copy link
Collaborator

sfackler commented Aug 8, 2021

"Failed to find path" does not seem to be a very helpful message when the actual problem is a non-UTF8 path.

@yaleman
Copy link
Author

yaleman commented Aug 9, 2021

Turns out there was another one I was hitting, so I've updated that as well.

@alexcrichton
Copy link
Owner

Are these unwraps getting hit frequently in practice? I think it's fine to get a better error message for when they are hit but if it's frequent then I'd want to fix the underlying issue ideally.

@yaleman yaleman changed the title Handling unwrap path errors in sanitize_sh Handling unwrap errors in lib.rs Aug 9, 2021
@yaleman
Copy link
Author

yaleman commented Aug 9, 2021

I had the issue, and looking in the issues for this project, it looks like the following tickets are related: #30, #47, #94.

My issue was that I was missing make, which isn't mentioned in the error anywhere previously. The UTF path thing is much less of an issue than the error handling in command.status().unwrap(); so I've updated the title of the PR.

src/lib.rs Outdated
let status = match command.status() {
Ok(value) => value,
Err(error) => {
panic!("Error running command {:?} => {:?}", command, error);
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think the command needs to be printed again because it was already printed a few lines above this?

Copy link
Author

Choose a reason for hiding this comment

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

Done

None => {
panic!("Failed to convert path to string: {:?}", &path);
}
};
Copy link
Owner

Choose a reason for hiding this comment

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

Could this and the above to_str use a helper function instead of duplicating?

Copy link
Author

Choose a reason for hiding this comment

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

Probably

@yaleman yaleman closed this by deleting the head repository Oct 10, 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