-
Notifications
You must be signed in to change notification settings - Fork 18
add simple Filter trait for composable filters #100
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
can be used by renderer to detect start and end of document
avoid infinite recursion for recursive calls:
error[E0275]: overflow evaluating the requirement `&mut jotdown::WriteAdapter<File>: std::fmt::Write`
|
= note: required for `&mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut ...` to implement `std::fmt::Write`
push/write single event at a time instead of an iterator of all events in a document
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum Container<'s> { | ||
| Document, |
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 like the idea of a document start and end as events.
| } | ||
| } | ||
|
|
||
| pub trait Filter<'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.
IMO this is a trait is not necessary. "filtering" or "mapping" are just instances of Render, just like https://doc.rust-lang.org/std/iter/struct.Map.html and https://doc.rust-lang.org/std/iter/struct.Filter.html are just instances of Iterator.
When I implement some kind of a custom filter, let's say struct RickRollMe<R>(R) where R: Renderer, I'd probably impl:
trait RickRollMeRenderExt : Render {
fn with_rickroll_me(self) -> RickRollMe<Self> { /* ... */ }
}
impl RickRollMeRenderExt for R where R: Render { /* ... */ }to make composition easier, so I can have:
html::Renderer::minified().with_rickroll_me()I also though about some kind of a
trait RenderWithOutput : Render {
type Output;
fn into_output(self) -> Self::Output;
fn render_into_document(self, input: &str) -> Result<Self::Output, <R as Render>::Error> {
self.render_document(input)?;
Ok(render.0.into_output())
}
}in jotdown, so that I can:
impl<R> RenderWithOutput for RickRollMe<R> where R : RenderWithOutput {
type Output = R::Output
fn into_output(self) -> R::Output {
self.0.into_output()
}
}and then the whole thing can:
let html = html::Renderer::minified().with_rickroll_me().render_into_document(input)?;so it all nicely composes.
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.
RickRolMeExt here needs to be defined as a side thing for every filtering functionality struct, but this is exactly like e.g. https://docs.rs/itertools/latest/itertools/trait.Itertools.html works.
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.
IMO this is a trait is not necessary. "filtering" or "mapping" are just instances of
Render, just like https://doc.rust-lang.org/std/iter/struct.Map.html and https://doc.rust-lang.org/std/iter/struct.Filter.html are just instances ofIterator.
The Render::with_filter here is similar to Iterator::filter except that it takes an object instead of a closure, FilteredRenderer impls Render just like iter::Filter impls Iterator.
The filter trait could potentially be replaced by a closure but one tricky part is that it needs to be generic for every W, which I am not sure if you can express in current Rust. Might need the unstable non_lifetime_binders feature for that?
Another reason to use an object is that it is then very easy to add state, i.e. instead of capturing mutable variables you can just add some fields on your struct.
When I implement some kind of a custom filter, let's say
struct RickRollMe<R>(R) where R: Renderer, I'd probably impl:trait RickRollMeRenderExt : Render { fn with_rickroll_me(self) -> RickRollMe<Self> { /* ... */ } } impl RickRollMeRenderExt for R where R: Render { /* ... */ }to make composition easier, so I can have:
html::Renderer::minified().with_rickroll_me()
It is unfortunate that you need both a trait and a trait implementation for every filter. I would prefer to minimize the amount of boilerplate per filter, as that is something that the user will implement themselves rather than being provided as with the itertools crate.
I also though about some kind of a
trait RenderWithOutput : Render { type Output; fn into_output(self) -> Self::Output; fn render_into_document(self, input: &str) -> Result<Self::Output, <R as Render>::Error> { self.render_document(input)?; Ok(render.0.into_output()) } }in
jotdown, so that I can:impl<R> RenderWithOutput for RickRollMe<R> where R : RenderWithOutput { type Output = R::Output fn into_output(self) -> R::Output { self.0.into_output() } }and then the whole thing can:
let html = html::Renderer::minified().with_rickroll_me().render_into_document(input)?;so it all nicely composes.
Why does this need to implemented for RickRollMe, isn't the filter fully agnostic of the output?
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.
The Render::with_filter here is similar to Iterator::filter except that it takes an object instead of a closure,
If I am to implement a "filter", I can just implement Render. The "filter" concept here is kind of a misnomer, because in practice the stuff I'm thinking about is going to be doing all sorts of transformations. Adding, substituting, removing events.
I would prefer to minimize the amount of boilerplate per filter
That is not something to optimize for. Even though I'm pushing for this functionality, I'll probably have 3 or 4 structs implementing some sort of transformations over djot events total. I'll have a single trait RenderExt { ... } with a method for every transformation, that just makes the syntax of chaining wrappers easier. It's not a big deal.
trait RenderWithOutput
RenderWithOutput is separating the "renders to some string/writer" trait from the core Render trait. So RickRollMe<R> where <R> produces a String, is also producing a string. And if R doesn't produce an output then RickRollMe also doesn't produce any output.
I think this is addressing your concern fro another comment:
I'm worried that having a too general trait will not let you gain much by having the trait.
The functionality of rendering and producing output should be separate. So then it's possible to express and compose both renderes that do not produce any outputs and ones that produce output.
I'm a bit uncertain if it's better to have trait RenderWithOutput : Render, or completely separate trait RenderingOutput (so then Render + RenderingOutput can be just combined), but it's a bit of a detail that can be done both ways AFAICT.
No description provided.