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

Remove panic in print_tty and return a Result #114

merged 2 commits into from Aug 26, 2019


Copy link

@phsym phsym commented Aug 25, 2019

As suggested in #103, printstd should avoid panicking.

Thus, as an improvement, printstd now ignores errors, but doesn't return the printed table size anymore.

print_tty now returns a Result and can be used for better control.

Closes #103

Thus, as an improvement, `printstd` now ignores errors, but don't return the printed size anymore.
`print_tty` now returns a `Result` and can be used for better control.
@phsym phsym added this to the V0.9.0 milestone Aug 25, 2019
Copy link

codecov bot commented Aug 25, 2019

Codecov Report

Merging #114 into master will increase coverage by 0.28%.
The diff coverage is 68.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #114      +/-   ##
+ Coverage   84.11%   84.39%   +0.28%     
  Files           5        5              
  Lines        1177     1173       -4     
  Hits          990      990              
+ Misses        187      183       -4
Impacted Files Coverage Δ
src/ 85.58% <ø> (ø) ⬆️
src/ 91.01% <100%> (ø) ⬆️
src/ 94.5% <100%> (ø) ⬆️
src/ 70.31% <100%> (ø) ⬆️
src/ 85.96% <25%> (+0.74%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e06ea7...2040ec8. Read the comment docs.

Copy link

I like that print_tty will return a Result.

As for the printstd method, I think exposing the error to the caller is best, so I would not use printstd at all. I guess I don't like either a) panic internally or b) hiding the error.

However, since you've changed the examples to use print_tty, I think it will help a lot of people trying to get started.

Copy link
Owner Author

phsym commented Aug 25, 2019

Well, printstd follows the philosophy of println! macro, silently ignoring errors when printing is not possible. But I guess I could also deprecate printstd with a message advising to use print_tty instead

@phsym phsym merged commit 6bb234c into master Aug 26, 2019
@phsym phsym deleted the printstd-nopanic branch August 26, 2019 20:00
Copy link

wfxr commented Mar 19, 2021

@phsym Would you mind releasing a new version to cover this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

Panic when piped into head
3 participants