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

Use error from stderr if stdout is empty #63

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

adumbidiot
Copy link
Contributor

I believe this fixes #24.

@fenhl
Copy link

fenhl commented Apr 13, 2022

Indeed it does.

Copy link
Collaborator

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It would be really nice to have tests for this behavior, but this change is obviously correct so I'll go ahead and merge for now :) let me know if you're interested in following up.

@jyn514 jyn514 merged commit f24188b into holmgr:master Aug 12, 2022
@adumbidiot
Copy link
Contributor Author

Honestly, I wasn't sure why stdout is involved at all when it seems the stderr always has the error (from my testing). However, I assumed that this isn't always the case, as the person who wrote this block probably got an error from the stdout since thats the way they wrote that. Also, I am not sure how I would write a test for this. Would we somehow create a project with an error for sweep to find?

@adumbidiot adumbidiot deleted the issue-24 branch August 12, 2022 22:40
@jyn514
Copy link
Collaborator

jyn514 commented Aug 12, 2022

Would we somehow create a project with an error for sweep to find?

That was my idea, yeah :) You should be able to reproduce it consistently by adding a custom rustc shell script in PATH that always gives an error.

It looks like cargo-sweep only has unit tests currently, not integration tests, so we'd need to set those up - in deadlinks I use assert_cmd which works pretty well without much boilerplate: https://github.com/deadlinks/cargo-deadlinks/blob/73de1dfcc24c6dcb2743505c176ff96aff9ca28d/tests/broken_links.rs#L8-L28
Let me know if that sounds like a hassle and I can add this first test, and hopefully it will be easier to add more in the future :)

@adumbidiot
Copy link
Contributor Author

Sorry, not sure if I have the time for that in the next few weeks. I'll try to implement that though if this test is still missing when I get some free time.

@fenhl
Copy link

fenhl commented Sep 5, 2022

Could we get a crates.io release with this fix please?

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.

Improve error handling
3 participants