JunoLab / Weave.jl

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

Improvements to the tex rendering pipeline #362

Closed JonasIsensee closed 4 years ago

JonasIsensee commented 4 years ago

This small PR relaxes some type restrictions and implements a few small helper functions in the tex rendering pipeline.

aviatesk commented 4 years ago

nice, sorry I'll be busy until weekend but I'll make sure to review on this then. If you have any higher view question, I will try to respond quickly though.

aviatesk commented 4 years ago

We can update our test when you think this PR is ready.

JonasIsensee commented 4 years ago

A more general thought:

Fixing these things I always have to consider what to do with the Tex format that uses juliacode as an environment. In itself it is not functional and I am not sure if there is anyone using it.

I propose to remove it from the package. That will make the source code shorter and more maintainable. (After all - you can't really test something that is non-functional) And once the API has been refactored we could bring it back as an example in the docs on how to define your own custom rendering type.

aviatesk commented 4 years ago

I propose to remove it from the package. That will make the source code shorter and more maintainable. (After all - you can't really test something that is non-functional) And once the API has been refactored we could bring it back as an example in the docs on how to define your own custom rendering type.

yeah, this sounds totally fine. Or we can just make Tex works like JMarkdown2tex except it doesn't generate the final PDF document but just stays as a .tex file

JonasIsensee commented 4 years ago

yeah, this sounds totally fine. Or we can just make Tex works like JMarkdown2tex except it doesn't generate the final PDF document but just stays as a .tex file

This is actually precisely what md2tex does. https://github.com/JunoLab/Weave.jl/blob/5c853150c872099610bb0a1d4dcc6504122e1330/src/rendering/texformats.jl#L154-L155

aviatesk commented 4 years ago

ah, yeah, I just thought about the "dispatch-base" external doc generation, but for now let's just remove Tex format

JonasIsensee commented 4 years ago

Ah, that's probably true. Generally this frees up the name tex. There is no real need for the standard tex pipeline JMarkdown2tex to have such a long name.

I imagine one could implement a

struct PDF <: WeaveFormat
    intermediate_format::TexFormat
    shell_cmd::String
end

that dispatches such that first the inner docformat is rendered, and then runs xelatex or whatever.

aviatesk commented 4 years ago

Okay, cool, so do you feel this is ready for review/merge again ? Then I will look into details agains and update tests, etc.

JonasIsensee commented 4 years ago

Renaming JMarkdown2tex and introducing a PDF struct may be the topic of another PR.

This one didn't change too much so I think it is ready for review/merge.

aviatesk commented 4 years ago

fwiw, merging (or rebasing) master will get rid of legacy test suites and enable much faster gh-action CI.

JonasIsensee commented 4 years ago

oops, I broke the tests again.

codecov-commenter commented 4 years ago

Codecov Report

Merging #362 into master will decrease coverage by 1.53%. The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
- Coverage   70.89%   69.35%   -1.54%     
==========================================
  Files          23       23              
  Lines        1254     1263       +9     
==========================================
- Hits          889      876      -13     
- Misses        365      387      +22     
Flag Coverage Δ
#unittests 69.35% <60.00%> (-1.54%) :arrow_down:
Impacted Files Coverage Δ
src/rendering/rendering.jl 88.88% <ø> (ø)
src/rendering/texformats.jl 66.31% <57.14%> (-22.19%) :arrow_down:
src/Weave.jl 46.00% <100.00%> (+0.54%) :arrow_up:
src/run.jl 80.08% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bd447ec...092adb6. Read the comment docs.