JunoLab / Weave.jl

Scientific reports/literate programming for Julia
http://weavejl.mpastell.com
MIT License
830 stars 96 forks source link

Introducing a SecondaryFormat for external rendering #379

Closed JonasIsensee closed 4 years ago

JonasIsensee commented 4 years ago

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, orPandocHTML` 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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

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 commented 4 years ago

Will do as soon as I find the time!

JonasIsensee commented 4 years ago

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