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

Handle unsuccessful exit status in main.rs. #32

Merged
merged 3 commits into from Jul 16, 2021
Merged

Conversation

apogeeoak
Copy link
Contributor

It may be better to encode an unsuccessful exit status in the Err variant instead of within the Ok variant. io::Result<ExitStatus> can be changed to io::Result<()> with an io::Error being created from ExitStatus. Another option is to create a custom error type. Most code, including main.rs, only checks the Err variant, assuming Ok was successful.

I can work on this change in another pull request if desired. This would be a breaking change but I think would result in more idiomatic code.

@Byron
Copy link
Owner

Byron commented Jul 16, 2021

Thanks a million!

I can work on this change in another pull request if desired. This would be a breaking change but I think would result in more idiomatic code.

Since the example code didn't manage to get its error handling correct, I think this is where I hope you will find the time to make the fix as suggested in a way that you see fit. If you would prefer a custom error type, implementation by hand should be fine, but so would pulling in quick-error.

My plan would be to create a new patch release once #33 is merged and a major release once the next PR has landed.

@Byron Byron merged commit 81d8c40 into Byron:main Jul 16, 2021
@apogeeoak apogeeoak deleted the exit_status branch July 16, 2021 16:10
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.

None yet

2 participants