Change write_to_html to allow fmt::Write

[stepantubanov]

Previous attempt:

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

In this version I've just added an associated type on StrWrite so that io::Error isn't lost. Let me know if that makes sense.

Other alternatives: Instead of exposing StrWrite, IoWriter and FmtWriter we can expose two functions: write_html_io and write_html_fmt that accept io::Write and fmt::Write (push_html can be removed then).

NOTE. This is a breaking change (should this PR bump version?) Changed base to 0.11

[Martin1887]

This must be reviewed and analyzed, but yes, it is a breaking change, and yes, it means an increment of the version number and indeed it is planned for the 0.11 milestone (#492).

Also, this pull request should be created against the branch 'branch_0.11', that is the branch where all breaking changes for the 0.11 version are being merged.

Thanks!

[Martin1887]

Interesting approach, it seems the only inconvenience is having to manually create a IoWrapper or a FmtWrapper...

And I think it could be solved implementing StrWrite for fmt::Write and io::Write. Also, the build seemed to fail.

[stepantubanov]

And I think it could be solved implementing StrWrite for fmt::Write and io::Write

Do you mean impl<T: fmt::Write> StrWrite for T and impl<T: io::Write> StrWrite for T? That doesn't work since these impls are conflicting.

Also, the build seemed to fail.

Fixed and rebased.

[Martin1887]

I see, so this seems OK for me, but I would like considering other options and the opinion from other reviewers before merging it.

Thanks!

[ollpu]

API-wise I definitely prefer the idea of just having separate entry functions write_html_io/fmt. Having to use the name write_html_fmt for strings (the most common case) is a little obtuse. Maybe we could keep push_html anyway. Although it is technically redundant, the name is a bit clearer and keeping it would reduce breakage.

Internally using a trait with an associated error type for writing looks good to me.

[notriddle]

I agree with @ollpu. Less complex API with less surface area is better.

[ollpu]

Great!

It would be even better if we didn't have to expose StrWrite and the wrappers in pulldown-cmark-escape. I guess one way to do it would be to have structs that impl Display, e.g. EscapeHtml, and always using write!() or similar. That's something for a later PR though.

[Martin1887]

Perfect, merge! Adding the EscapeHtml and co structs seems a good point for the 0.11 release, so I think they should be added before releasing it.

The other remaining issue for the 0.11 is #67.

[Martin1887]

I'm trying to hide StrWriter and the associated structs but I'm not getting it without duplicating the code for the methods of HtmlWriter, since the user has to specify the type, @ollpu.

Respect to the EscapedHtml and so structs implementing Display, they are easy to implement but they would have to write in an empty String buffer, and then write that String into the writer of HtmlWriter, so it would have a performance penalty. Please correct me if I'm wrong.

My implementation for those structs (example only with EscapedHtml but EscapedHref and EscapedHtmlBodyText are identical):

pub struct EscapedHtml<'a> {
    html: &'a str,
}
impl<'a> fmt::Display for EscapedHtml<'a> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        let mut buffer = String::new();
        escape_html(&mut buffer, self.html)?;
        write!(f, "{}", buffer)
    }
}

[ollpu]

@Martin1887

On the first point, to be able to hide StrWrite from public API, it needs to be moved (or duplicated) to the pulldown-cmark crate. It should be fine for HtmlWriter to use it, since it isn't public. Tell me if I'm missing something here. I can try doing this myself later too.

For the struct Display approach, you can do this with the current StrWrite and FmtWriter:

pub struct EscapedHtml<'a> {
    html: &'a str,
}
impl<'a> fmt::Display for EscapedHtml<'a> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        escape_html(FmtWriter(f), self.html)
    }
}

The internal functions could just take &mut fmt::Formatter directly, or a simpler trait, instead of StrWrite, though.

[Martin1887]

I would leave as public the trait and the structs in the pulldown_cmark_escape crate, since it is more low-level, instead of duplicating them in pulldown_cmark.

But this approach would not avoid the use of FmtWriter and IoWriter in the write_html function, right? Or at least it could not be run over an io::Write struct. Also, I think using the EscapedHtml and co structs in this function would have a performance penalty due to the double write in a temporal FmtWriter in the format call and then another time in the internal writer field (the only one done right now).

But I'm probably missing something...

[stepantubanov]

@Martin1887

Also, I think using the EscapedHtml and co structs in this function would have a performance penalty due to the double write in a temporal FmtWriter in the format call and then another time in the internal writer field (the only one done right now)

Hm, these simple forwarding calls almost certainly should be zero cost and either get inlined to where they're called or inline internal writer call into them (whatever compiler decides based on heuristics).

[ollpu]

@Martin1887 If the escaping works through Display impls, then it can be used with either fmt or io writers by using write!(writer, "{}", EscapedHtml(x)) (it'll go through write_fmt in StrWrite).

There's definitely more for the optimizer to chew on, and I'm also vary of having extra layers of write! (I hear they don't always optimize away). If you don't think it's worth it, let's keep it as is. I don't think we should have the Display structs if we don't use them ourselves.

And yes, something similar to StrWrite is needed in pulldown-cmark/src/html.rs either way to abstract over io/fmt.

[Martin1887]

I think it is worthy for an easier interface to escape HTML strings, and we could even using them in the html.rs functions if the performance penalty is low, but I couldn't see how them could avoid the exposure of StrWrite, FmtWriter and IoWriter 😄 .

Could you create the pull request? Thanks!