Scrabsha/html errors

[scrabsha]

No description provided.

[myrrlyn]

In practice I don't know if it actually matters (I am assuming that you've run tests and observed it work; I haven't tried to work on the compiler before and so haven't actually downloaded or run this myself), but I think this clause loses the current .next() segment if it is not able to write into self.inner, so a retry would skip it. If it works as-is, then I wouldn't worry about it.

[scrabsha]

Actually it is not a problem if we don't write the current .next(), as long as we exit early and return the correct amount of bytes we actually wrote.

But the good news is that you're highlighting the fact that we should not return an error if we wrote something to the sink. I do need to fix that.

[myrrlyn]

Okay so I've played with writing this as a standalone module and while I have some minor practical differences as a result of our prior conversation and seeing your solution first, ultimately I don't think I have anything worth bringing up as an actual change. The Peekable wrapper could be used instead of a local Option, or you could hold the original &str or &[u8] and pull out the SplitInclusive behavior we actually want (.position() and .filter().map()), but these don't affect the API.

I refuse to bikeshed this to a standstill before it even hits rustc. It's behavior I want the compiler to do and you made the compiler do it. Excellent. I think there could be some internal simplification (for instance, I got by without an EscapedHtml enum, and just used (src_bytes_consumed, segment): (usize, &[u8])) but, again, that's getting deep into the weeds when the important part is that it correctly emits text that is safe to place directly into an HTML template.

[scrabsha]

@myrrlyn

Thank you so much for taking time to review this piece of code. It's much appreciated.

I tried to remove the remove the trimmed_escaped field and make iter a Peekable<SplitInclusive<...>>, but failed. I think my level in Rust is not good enough to write it cleanly.

I'm wondering how you removed the EscapedHtml enum: how do you know if you must call write or write_all in the Write implementation? Could you post a playground link or something? I don't think I'll implement it though, as the goal is also to stay simple and easy to review.