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 std::fmt::Write instead of custom StrWrite trait #493

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

camelid
Copy link
Contributor

@camelid camelid commented Oct 9, 2020

Closes #492.

Copy link
Collaborator

@marcusklaas marcusklaas left a 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(()),
Copy link
Collaborator

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(()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto.

@camelid
Copy link
Contributor Author

camelid commented Oct 11, 2020

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.

Yeah, I agree :/

Maybe not worth doing for now.

@camelid
Copy link
Contributor Author

camelid commented Oct 11, 2020

@edward-shen
Copy link
Contributor

From that topic:

simulacrum: It is perhaps worth noting that io::Write::write_fmt does preserve the io Error in the raw form (https://doc.rust-lang.org/nightly/src/std/io/mod.rs.html#1498-1513)
simulacrum: so if you're using write! or similar with io sources, and not the fmt trait directly, you'll get the error

From my understanding, our use case falls under the former category, so is this still considered a blocker?

@leeola
Copy link

leeola commented Jul 2, 2023

I wonder if this PR could be extended to allow a more generic representation of push_html(&mut String,..)? I'm running into a related problem where a template library (Sailfish) has a Buffer type that impls fmt::Write, but not io::Write for similar reasons (i assume) that this library originally wrote StrWrite.

As far as i can tell this requires users of Sailfish to allocate a secondary String to receive html from this lib. Making the push_html generic seems a good way to deal with this, and fmt::Write seems a possible candidate to support that with this PR. Thoughts?

@Martin1887
Copy link
Collaborator

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.

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.

Use core::fmt::Write instead of custom StrWrite?
5 participants