-
Notifications
You must be signed in to change notification settings - Fork 18
chore: introduce event-based Renderer #101
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
base: master
Are you sure you want to change the base?
Conversation
d94131b to
eaf107b
Compare
| } | ||
|
|
||
| struct Writer<'s, 'f> { | ||
| indent: &'f Option<Indentation>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kind of easier to just pass it as an argument to functions than pile lifetimes on Writer.
5f6853c to
290ccc4
Compare
| } | ||
|
|
||
| #[test] | ||
| fn rickroll_me() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this effort to pull out pranks.
|
|
||
| fn emit(&mut self, event: Event<'s>) -> Result<(), Self::Error> { | ||
| match event { | ||
| Event::Start(Container::Link(_link, t), attrs) => self.0.emit(Event::Start( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, BTW. Is it necessary for End events to carry all this stuff again? It just makes so that filters will need to keep track of stuff and substitute it twice (once in Start, once in End to be technically correct), and it increases amount of data in the events, while if necessary it is probably easier to track such state once in the final html renderer, if it is even using it for anything (here look like it does not).
My understanding is that in html all the extra data is rendered on the start of the container, and end of the container is typically just </foo> without much need for anything but the type of the event.
Is it primarily caused by the Start and End using same Container type? Maybe there could be just Event::Start(ContainerStart) and Event::End(ContainerEnd)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some renderers could potentially write the data at the end of the container, e.g. if rendering djot/markdown links. However, the attributes are only on the start event so those have to be saved until the end event anyway.
But I agree it is not ideal that the data should exist in both events while it works to have it on only one of them depending on the renderer.
Not sure how it affects performance to have duplicate data, the Events do not have any heap data (unless added by filter) so I don't expect much difference.
A benefits with having only one type is that there one less almost similar type to deal with, and the start and end types are actually bound together with the type system.
hellux
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conflicts a bit with #100, any thoughts on the changes there?
| pub fn with_io_writer<NewWriter>( | ||
| self, | ||
| output: NewWriter, | ||
| ) -> Renderer<'s, WriteAdapter<NewWriter>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I agree that the Render trait should not have any requirement on the output type, we then e.g. lose the WriteAdapter helper and each renderer has to implement it on its own like here.
do you have an example where you need to render to an output that doesnt implement fmt::Write or io::Write?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my mind the "rendering" is just consuming events. What exactly happens is kind of not a concern of caller of the Renderer. Maybe the render doesn't even have a writer. Maybe it is counting number of letters "a" or something and want to return TooManyLettersA. I can think of renderers that check security policy and want to error out with invalid markup detected and so on. That's why I think Render::emit just has to have a generic error. A caller renders to a concrete Renderer, on error they will be handling it.
Regrettably, this is not how std::io traits are, because trait associated types were not a thing yet when Rust 1.0 was released. The whole issue is a completely mess now, and WritAdapter is a clever workaround to unify fmt::Error with io::Error.
lose the WriteAdapter helper and each renderer has to implement it on its own like here.
Fundamentally, 95% of jotdown is the parser + html renderer. :D . There are not going to be many alternative leaf-Render impls, and if they are they are often not going to be outputting to a writer, but doing something weird. And if they are outputting they might be using async and AsyncWrite or something, which is even more incompatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my mind the "rendering" is just consuming events. What exactly happens is kind of not a concern of caller of the Renderer. Maybe the render doesn't even have a writer. Maybe it is counting number of letters "a" or something and want to return TooManyLettersA. I can think of renderers that check security policy and want to error out with invalid markup detected and so on. That's why I think Render::emit just has to have a generic error. A caller renders to a concrete Renderer, on error they will be handling it.
I'm worried that having a too general trait will not let you gain much by having the trait. The current Render trait is already quite shallow as is and doesn't provide much value except:
- providing a unified interface that can be shared by renderers, making it easier to both use and implement renderers,
- allowing code that writes to e.g. a file be agnostic of the output format (renderer),
- providing the WriteAdapter helper to allow any renderer to support both fmt::Write and io::Write without any extra effort.
but most of this requires that the interface is "take events and dump to a Write object" and wouldn't work if it was "take events and do anything with it".
Fundamentally, 95% of jotdown is the parser + html renderer. :D . There are not going to be many alternative leaf-Render impls, and if they are they are often not going to be outputting to a writer, but doing something weird. And if they are outputting they might be using async and AsyncWrite or something, which is even more incompatible.
One goal is to try to follow the aim of djot to be agnostic to the output format (not exclusively html), so I do at least want to encourage for renderers to be implemented for alternative formats, and one way is to provide some simple guidance/helpers with the Render trait.
| fn finish(&mut self) -> Result<(), Self::Error>; | ||
| } | ||
|
|
||
| pub trait RenderExt<'s>: Render<'s> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason to have a separate trait that requires an additional import instead of just adding these default impls to Render?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of a purist on the "extra methods with default impl don't belong on the trait itself". Ext traits allow e.g. taking self by value (self: Sized) without breaking the dyn-compatibility of the original trait.
As for the UX of the importing extra trait - it is a bit unfortunate, but in practice I think typical downstream users will want to use utility methods on html::Renderer itself, as it's just even easier. So I consider the methods here, can of ... half-internal utility methods.
BTW. One thing when I deviate in philosophy from the existing API design, is that I think the Parser should be largely pushed into background. Most users probably have a whole document as a &'str, and want to get it rendered whole to String, or to some writer. The utility methods working on Parser as Iterator, and even bring up the Parser altogether, should be an "advanced" use case. So e.g. the introductory example from https://docs.rs/jotdown/0.9.0/jotdown/#examples should become just
let djot_input = "hello *world*!";
let html = jotdown::html::render_to_string(djot_input);
assert_eq!(html, "<p>hello <strong>world</strong>!</p>\n");
and mild customizing version should be:
let djot_input = "hello *world*!";
let html = jotdown::html::Renderer::intended(/* ... */).render_to_string(djot_input);
assert_eq!(html, "<p>hello <strong>world</strong>!</p>\n");
No need to bring up Parser and events anywhere, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the UX of the importing extra trait - it is a bit unfortunate, but in practice I think typical downstream users will want to use utility methods on html::Renderer itself, as it's just even easier. So I consider the methods here, can of ... half-internal utility methods.
One purpose of the Render trait is try to unifiy the interface between renderers so I'm not sure I agree that the main interface should be determined by each renderer implementation.
BTW. One thing when I deviate in philosophy from the existing API design, is that I think the Parser should be largely pushed into background. Most users probably have a whole document as a &'str, and want to get it rendered whole to String, or to some writer. The utility methods working on Parser as Iterator, and even bring up the Parser altogether, should be an "advanced" use case. So e.g. the introductory example from https://docs.rs/jotdown/0.9.0/jotdown/#examples should become just
let djot_input = "hello *world*!"; let html = jotdown::html::render_to_string(djot_input); assert_eq!(html, "<p>hello <strong>world</strong>!</p>\n");and mild customizing version should be:
let djot_input = "hello *world*!"; let html = jotdown::html::Renderer::intended(/* ... */).render_to_string(djot_input); assert_eq!(html, "<p>hello <strong>world</strong>!</p>\n");No need to bring up Parser and events anywhere, IMO.
I agree that if there is a good interface for implementing filters we can try to make the renderer/Render trait the main interface and let that create the parser for you. Events are still the main interface for filtering, but for the example without any filtering it can be omitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One purpose of the Render trait is try to unifiy the interface between renderers so I'm not sure I agree that the main interface should be determined by each renderer implementation.
The Render is an interface between the renderers. So one Renderer can take another R : Render and wrap it around, just like e.g. Iterators impl do. I don't think it is meant for the consumer itself.
It is really unfortunate that Rust does not have something like "inherent traits" (like https://internals.rust-lang.org/t/pre-pre-rfc-using-inherent-impls-to-complete-trait-impls/3545/18 I think, can be mimicked with: https://docs.rs/inherent/latest/inherent/), and placing "general purpose wrappers" requires another trait that then needs to be imported, but 🤷 .
E.g. render_to_string does not belong to trait Render, because it is not Renderer job to change how the document is actually render as whole. And if render_to_string is to consume self, then it can't be on Render because that breaks dyn-compatibility.
|
|
||
| pub trait RenderExt<'s>: Render<'s> { | ||
| /// Parse and render the whole document with `renderer` | ||
| fn render_document(&mut self, src: &'s str) -> Result<(), Self::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a nice helper function now when filters are applied on the renderer and typically no step is needed between parsing and rendering (+filtering)
I have kind of a vision of a end goal mostly described: https://github.com/hellux/jotdown/pull/100/files#r2539532835 but in this PR I wanted to make a smallest change I can show to you for consideration. |
290ccc4 to
57d61b9
Compare
No description provided.