-
Notifications
You must be signed in to change notification settings - Fork 11
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
Render from borrowed events? #24
Comments
Hi! The jotdown format and this crate seems awesome, thanks!
I'd like to be able to create multiple html outputs from the same
jotdown file, by finding different parts of the event stream and
rendering them separately.
Note that jotdown is just a parser/renderer implementation, the
markup language itself is called djot.
```rust
let jd = jotdown::Parser::new(&read_to_string(path)?).collect::<Vec<_>>();
let part_a: &[&Event] = some_filter(&jd);
let html_a = String::new();
jotdown::html::push(part_a.iter(), &mut html_a);
let part_b: &[&Event] = other_filter(&jd);
let html_b = String::new();
jotdown::html::push(part_b.iter(), &mut html_b);
```
This fails because iterating a slice can only give an iterator of
borrowed events, not owned events.
It could be fixed by using `part_a.iter().cloned()`, but `Event`
currently don't implement `Clone`.
Oh, I thought `Event` implemented `Clone` but it does not, I don't see a
reason why it shouldn't. This should be fixed!
I would guess the performance impact would be small from cloning, almost
none of the events have data on the heap.
Another alternative would be to perform the parsing twice, but that
would sacrifice time for reduced memory footprint.
If the two parts are completely disjunct, it might be possible to
transfer the ownership of the `Event`'s, but it depends on what type of
filter you need.
But I don't really see a reason that formatting html should require
ownership of the events, so I think it should be relatively easy to
add formatting of borrowed events.
It is true that the renderer does not necessarily need ownership of the
`Event`'s to render them, as it is only reading them. But the `Parser`
returns an Iterator that creates and owns each `Event`, one at a time.
And I don't think the iterator can lend from itself so it must transfer
the ownership of the `Event` to the renderer.
In your case, the events are owned and stored by the jd Vec outside the
iterator and in that case it works. But we also want to be able to take
events directly from the parser and render them immediately, avoiding
storing all `Event`s in a large buffer.
I am not entirely sure, but I think this is a limitation of the
language. It should be possible to store a single Event in the iterator
and temporarily lend it to the renderer before overwriting it with the
next one, but I'm not sure if it is possible to express the lifetimes
for this. I think the work for this is ongoing and it might be possible
in future versions of Rust.
If there is agreement that this would be useful, I'm open for
attempting to write a PR implementing it.
I am not sure exactly how to solve this right now. We need to support
rendering directly from the parser, at least. It might be possible to
modify the API to support both cases somehow? E.g. extracting the
rendering of a single borrowed event to a separate function and
providing two entry points, one that borrows and one that owns?
For now, I pushed a commit that makes the `Event`'s clonable, so it is
possible to use that right now if you use the `master` branch.
|
Thanks, cloning works fine for me (at least for now). I might attempt to make a make it possible to use references directly later, I think it should be possible to take an Feel free to tag this issue for handling later, or just closing it as you see fit. |
I did some experimentation on a branch, pub trait Render {
fn render_event<'s, W: fmt::Write>(&mut self, e: &Event<'s>, out: W) -> std::fmt::Result;
fn render_epilogue<W: fmt::Write>(&mut self, out: W) -> std::fmt::Result;
fn push<'s, I, W>(&mut self, mut events: I, mut out: W) -> fmt::Result
where
I: Iterator<Item = Event<'s>>,
W: fmt::Write,
{
events.try_for_each(|e| self.render_event(&e, &mut out))?;
self.render_epilogue(&mut out)
}
fn push_ref<'s, E, I, W>(&mut self, mut events: I, mut out: W) -> fmt::Result
where
E: AsRef<Event<'s>>,
I: Iterator<Item = E>,
W: fmt::Write,
{
events.try_for_each(|e| self.render_event(e.as_ref(), &mut out))?;
self.render_epilogue(&mut out)
}
fn write<'s, I, W>(&mut self, events: I, out: W) -> io::Result<()>
where
I: Iterator<Item = Event<'s>>,
W: io::Write,
{ .. }
fn write_ref<'s, E, I, W>(&mut self, events: I, out: W) -> io::Result<()>
where
E: AsRef<Event<'s>>,
I: Iterator<Item = E>,
W: io::Write,
{ .. }
} The renderer has to render each event separately, which slightly decreases its flexibility (although, a renderer could choose to implement push/push_ref manually also). There is a slight performance decrease, not sure why. The rendering is almost 10% slower (rendering is around 10% of parse time, so in the magnitude of 1%). Any thoughts on this API? |
allow rendering of iterators with borrowed events resolves #24
allow rendering of iterators with borrowed events resolves #24
allow rendering of iterators with borrowed events resolves #24
allow rendering of iterators with borrowed events resolves #24
allow rendering of iterators with borrowed events resolves #24
allow rendering of iterators with borrowed events resolves #24
allow rendering iterators with borrowed events resolves #24
Hi! The jotdown format and this crate seems awesome, thanks!
I'd like to be able to create multiple html outputs from the same jotdown file, by finding different parts of the event stream and rendering them separately. Something like this:
This fails because iterating a slice can only give an iterator of borrowed events, not owned events.
It could be fixed by using
part_a.iter().cloned()
, butEvent
currently don't implementClone
. But I don't really see a reason that formatting html should require ownership of the events, so I think it should be relatively easy to add formatting of borrowed events.If there is agreement that this would be useful, I'm open for attempting to write a PR implementing it.
The text was updated successfully, but these errors were encountered: