-
Notifications
You must be signed in to change notification settings - Fork 261
Use std::fmt::Write instead of custom StrWrite trait
#493
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
base: master
Are you sure you want to change the base?
Conversation
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.
The implementation looks good, but I just now noticed that fmt::Write has its own error. This means that it's no longer possible to pass along IO errors. If a write fails to a Write object, the user will have no way to discover why. This seems like a serious drawback of the change.
| (**self).write_str(s) | ||
| fn write_str(&mut self, s: &str) -> fmt::Result { | ||
| match self.0.write_all(s.as_bytes()) { | ||
| Ok(()) => Ok(()), |
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.
We could .map_err here.
| (**self).write_fmt(args) | ||
| fn write_fmt(&mut self, args: Arguments) -> fmt::Result { | ||
| match self.0.write_fmt(args) { | ||
| Ok(()) => Ok(()), |
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.
Ditto.
Yeah, I agree :/ Maybe not worth doing for now. |
|
@marcusklaas Not sure if you're on the Rust Zulip, but I posted this: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/fmt.3A.3AResult.20doesn't.20allow.20having.20an.20error.20message |
|
From that topic:
From my understanding, our use case falls under the former category, so is this still considered a blocker? |
|
I wonder if this PR could be extended to allow a more generic representation of As far as i can tell this requires users of Sailfish to allocate a secondary |
|
This is an old pull request and in draft state, it should be redesigned and thought again. Sorry, but this will not happen in the short term unless someone else begin to work in this. |
Closes #492.