Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Replace show image crate #55
Replace show image crate #55
Changes from 19 commits
3207970
d276162
838d98f
ca7be65
d15420a
d2143e7
1ed9b63
d17ea8e
c8eb1a0
8cce500
cac45e4
8bc828a
6a2a990
a914a0f
ee43fc9
6a27737
c82a4a3
a81c555
fd547bc
4aa37a6
6302790
beee7e2
86f7193
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for rust libraries it's generally not recommended to use strings for the error type. this makes it hard to do anything but log the error. I think other places in Ibeji are using
Status
(e.g. here) so ideally we should use that here tooThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used String to be consistent with the sdl2 crate's methods, which use String.
Status comes from the tonic crate and is currently only used in conjunction with gRPC-related calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting errors can be done with
map_err
. It's also worth mentioning that the library owner would prefer to use a proper error type but hasn't made the change due to concerns with breaking changes: Rust-SDL2/rust-sdl2#1053Then
Status
wouldn't be the right thing to use, we should find something else. Consider defining your own error type similar to what Freyja does, or you could useanyhow
(not recommended for libraries but easy to use and better thanString
) orthiserror
, or even reusestd::io::Error