Skip to content
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 trait #10

Closed
kmaasrud opened this issue Feb 9, 2023 · 5 comments
Closed

Render trait #10

kmaasrud opened this issue Feb 9, 2023 · 5 comments

Comments

@kmaasrud
Copy link
Collaborator

kmaasrud commented Feb 9, 2023

Djot is not meant to be HTML-centric, and jotdown should not be either. Lets let library consumers express anything that can render jotdown events via a Render trait (or similarly named.) Like the HTML implementation, we would have to distinguish between a unicode-accepting writer and a byte sink. The push and write names seem suitable:

pub trait Render {
    fn push<'s, I: Iterator<Item = Event<'s>>, W: fmt::Write>(events: I, out: W);
    fn write<'s, I: Iterator<Item = Event<'s>>, W: io::Write>(events: I, out: W) -> io::Result<()>;
}

The advantage of this approach over a module with standalone functions is obvious: Well-defined extensibility. This also lets libraries accept any rendering format in a more succinct way and additionally makes it clearer how one would implement their own renderer.

I'm debating whether we require a reference to self in the trait methods or not... One might want to allow extensions or rendering customisation, so might be better to include a reference.

@hellux
Copy link
Owner

hellux commented Feb 9, 2023

A benefit of having a trait could be to enable reuse of the adapter between std::io::Write/std::fmt::Write. However, here it looks like you need to implement both methods.

We have a html::Writer object that could potentially implement some trait, e.g:

pub trait Render {
    // implement this
    fn write_fmt<'s, I: Iterator<Item = Event<'s>>, W: std::fmt::Write>(
        &mut self,
        events: I,
        out: W,
    ) -> std::fmt::Result;

    // and get these for free
    fn push<'s, I: Iterator<Item = Event<'s>>, W: std::fmt::Write>(&mut self, events: I, out: W) {
        self.write_fmt(events, out).unwrap();
    }

    fn write<'s, I: Iterator<Item = Event<'s>>, W: std::io::Write>(
        &mut self,
        events: I,
        out: W,
    ) -> std::io::Result<()> {
        // adapter stuff here

        self.write_fmt(events, &mut output)
            .map_err(|_| output.error.unwrap_err())
    }
}

Then you have a mutable reference, so you can configure it if needed when constructing it.

@kmaasrud
Copy link
Collaborator Author

kmaasrud commented Feb 9, 2023

Yes, using the adapter is a great suggestion! Is there a reason why you are unwrapping the error in push? I guess it's since Djot parsing can't really error, but we may want to give renderers the possibility to throw an error either way. Maybe we only need push and write then, push being the required implementation?

@hellux
Copy link
Owner

hellux commented Feb 9, 2023

Is there a reason why you are unwrapping the error in push? I guess it's since Djot parsing can't really error, but we may want to give renderers the possibility to throw an error either way.

Yeah, I think the html::Writer does not cause formatting errors at least because it always uses the write! macro which is checked at compile time. But I guess there is nothing stopping the std::fmt::Write implementor from causing an error when we try to write to it. Usually it is just a string which should never cause an error on writing to it, but it could be anything, I guess? So I guess we should not unwrap, and just propagate the result instead.

Maybe we only need push and write then, push being the required implementation?

Yeah, that is a better idea, we should only need those two.

@kmaasrud
Copy link
Collaborator Author

kmaasrud commented Feb 9, 2023

Awesome! In that case, I'll draft this out tomorrow and open a PR where we can have a look at this.

@kmaasrud kmaasrud mentioned this issue Feb 10, 2023
@kmaasrud
Copy link
Collaborator Author

Closed by #12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants