Skip to content

Use std::fmt::Write instead of custom StrWrite trait#493

Draft
camelid wants to merge 1 commit into
pulldown-cmark:mainfrom
camelid:fmt-write
Draft

Use std::fmt::Write instead of custom StrWrite trait#493
camelid wants to merge 1 commit into
pulldown-cmark:mainfrom
camelid:fmt-write

Conversation

@camelid

@camelid camelid commented Oct 9, 2020

Copy link
Copy Markdown
Contributor

Closes #492.

@marcusklaas marcusklaas left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread src/escape.rs
(**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
Copy Markdown
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.

Comment thread src/escape.rs
(**self).write_fmt(args)
fn write_fmt(&mut self, args: Arguments) -> fmt::Result {
match self.0.write_fmt(args) {
Ok(()) => Ok(()),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ditto.

@camelid

camelid commented Oct 11, 2020

Copy link
Copy Markdown
Contributor Author

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

camelid commented Oct 11, 2020

Copy link
Copy Markdown
Contributor Author

@edward-shen

Copy link
Copy Markdown
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

leeola commented Jul 2, 2023

Copy link
Copy Markdown

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
Copy Markdown
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