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

Introducing a SecondaryFormat for external rendering #379

Closed
wants to merge 2 commits into from

Conversation

JonasIsensee
Copy link
Contributor

@JonasIsensee JonasIsensee commented Jun 14, 2020

Hi,
as speculated in my previous PR I implemented an idea to
make external rendering such as md2pdf or pandoc2pdf etc.
work just as a regular WeaveFormat.
A SecondaryFormat wraps a primary WeaveFormat that does the hard work.
In the very end of the weave function there is now a little call
postprocessing(doc, out_path)
that does nothing if the format is one of the normal formats but dispatches to the
external rendering if a LatexPDF, PandocPDF, or PandocHTML` has been passed.

@aviatesk
You seem to have been working on these files at the same time but I decided to create PR instead of first trying to fix the merge requests.
The current implementation already works quite well so maybe you could have a look at it to see if it is worth cleaning up.

@aviatesk
Copy link
Member

thanks @JonasIsensee , and sorry for I did a duplicated work.
I rewrite the most process after render_doc here, and the main idea is almost same as yours.

I found you added new mintedtex format in this PR and this would be really nice enhancement.
Would you mind add it with the current master, please ?
Maybe creating a separate PR would be good.

@JonasIsensee
Copy link
Contributor Author

JonasIsensee commented Jun 14, 2020

Actually I don't think too much of this is duplicate.

You currently have a type for e.g. md2pdf that is a duplicate of md2tex only that in the end it also calls latex. And you do the same thing for pandoc.

I implemented a wrapper instead.
That allows me to use LatexPDF(TexMinted()) and LatexPDF(JMarkdown2tex()) .

(or whatever the types are called now)

In my PR I implemented these secondary formats that delegate all the rendering to the primary format and then call the external process.

In my opinion that is more elegant because we don't have define e.g. TexMinted twice.

So maybe, if you agree, we could combine this idea with your work

@aviatesk
Copy link
Member

I see your idea, thanks for explaining in detail.
Well, I'm still not sure if I like the idea of adding another type hierarchy layer for our code base.
I agree with that it will enable us to remove the duplicated code, but it also adds another complexity (especially about indirections on format_hoge functions and property accessing, well I guess we actually don't need the latter for this to work ?).
So I'm not sure we should go this way, imho. Could I hear you idea from this point of view ? Do you think the code duplication removal from this secondary layer of format type hierarchy deservers its complexity ?

@JonasIsensee
Copy link
Contributor Author

I agree, I am also not fully comfortable with the additional layer of complexity of the wrapper.
On the other hand I also don't like the code duplication of the other approach.
In particular: The external rendering is optional and separate from everything else.
I find it very unsatisfactory that we would have to redefine everything (with all parameters and such)
only to be able to run a single additional command in the end.

A simpler solution would be to split the feature into a separate function altogether and have something like.

out_path = weave("mydoc.jmd", doctype="md2tex")
compile_latex(out_path)

out_path = weave("mydoc.jmd", doctype="pandoc")
compile_pandocpdf(out_path)

Another way of splitting this while keeping it in one function would be to do

weave("mydoc.jmd", doctype=WeaveLatex(), postprocess=LatexPDF())

@aviatesk
Copy link
Member

aviatesk commented Jun 15, 2020

Okay, thanks. Let's go adding this layer (i.e. the approach of this PR). I think defining something like your SecondaryFormat, which allows "add-on postprocessing" would be certainly elegant and better API.

@JonasIsensee , would you mind re-crafting a PR against the current master, please ??
The main differences are:

  • format_hoge functions are now called render_hoge, and they take ::WeaveFormat as a first arguments for clearer dispatch design (I forgot to change format_inline to accept ::WeaveFormat, though. They're better to be accept that as well)
  • the processes after render_doc for every format are now handled by write_doc API. It only writes the return value of render_doc into the target location for most formats, while formats with external programs use their own implementation of write_doc

I don't mind if you rename write_doc into post_process or something you prefer. We have writer folder for now, but change the name of it as well if you want.

@JonasIsensee
Copy link
Contributor Author

Will do as soon as I find the time!

@JonasIsensee
Copy link
Contributor Author

Opened the new version of this PR at #380 .
With your improvements I like it even better!

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

Successfully merging this pull request may close these issues.

2 participants