Change `write_to_html` to allow `fmt::Write` #870

stepantubanov · 2024-03-25T02:50:24Z

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 · 2024-03-25T07:30:23Z

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 · 2024-04-06T09:35:04Z

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 · 2024-04-12T19:59:10Z

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 · 2024-04-13T09:02:46Z

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.

pulldown-cmark-escape/src/lib.rs

Co-authored-by: Roope Salmi <rpsalmi@gmail.com>

notriddle · 2024-04-16T20:52:26Z

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 · 2024-04-17T14:46:42Z

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 · 2024-04-21T11:26:37Z

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 · 2024-04-21T12:45:31Z

@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 · 2024-04-21T14:47:38Z

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 · 2024-04-21T16:31:02Z

@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 · 2024-04-21T16:57:48Z

@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 · 2024-04-21T17:40:00Z

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!

stepantubanov force-pushed the write-to-fmt branch from 56fcf0e to 4a79f63 Compare March 26, 2024 13:59

stepantubanov changed the base branch from master to branch_0.11 March 26, 2024 13:59

stepantubanov force-pushed the write-to-fmt branch from 4a79f63 to 42b8274 Compare March 26, 2024 14:00

Martin1887 linked an issue Mar 30, 2024 that may be closed by this pull request

Use core::fmt::Write instead of custom StrWrite? #492

Closed

Martin1887 requested a review from notriddle April 6, 2024 09:48

Change write_to_html to allow fmt::Write

081a4d2

stepantubanov force-pushed the write-to-fmt branch from 42b8274 to 081a4d2 Compare April 12, 2024 19:59

Martin1887 requested a review from ollpu April 14, 2024 09:03

ollpu reviewed Apr 16, 2024

View reviewed changes

pulldown-cmark-escape/src/lib.rs Outdated Show resolved Hide resolved

Update pulldown-cmark-escape/src/lib.rs

00c9c48

Co-authored-by: Roope Salmi <rpsalmi@gmail.com>

do not reexport writer wrappers

cefabf3

notriddle approved these changes Apr 16, 2024

View reviewed changes

ollpu approved these changes Apr 17, 2024

View reviewed changes

Martin1887 merged commit 7a1e53c into pulldown-cmark:branch_0.11 Apr 17, 2024

ollpu mentioned this pull request Apr 22, 2024

Use structs that impl Display for escaping #881

Closed

ollpu mentioned this pull request Dec 15, 2024

Add basic skeleton for developer docs #988

Merged

Change write_to_html to allow fmt::Write #870

Change write_to_html to allow fmt::Write #870

Uh oh!

Conversation

stepantubanov commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Martin1887 commented Mar 25, 2024

Uh oh!

Martin1887 commented Apr 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stepantubanov commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Martin1887 commented Apr 13, 2024

Uh oh!

ollpu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

notriddle commented Apr 16, 2024

Uh oh!

ollpu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Martin1887 commented Apr 17, 2024

Uh oh!

Martin1887 commented Apr 21, 2024

Uh oh!

ollpu commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Martin1887 commented Apr 21, 2024

Uh oh!

stepantubanov commented Apr 21, 2024

Uh oh!

ollpu commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Martin1887 commented Apr 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Change `write_to_html` to allow `fmt::Write` #870

Change `write_to_html` to allow `fmt::Write` #870

stepantubanov commented Mar 25, 2024 •

edited

Loading

Martin1887 commented Apr 6, 2024 •

edited

Loading

stepantubanov commented Apr 12, 2024 •

edited

Loading

ollpu left a comment •

edited

Loading

ollpu left a comment •

edited

Loading

ollpu commented Apr 21, 2024 •

edited

Loading

ollpu commented Apr 21, 2024 •

edited

Loading