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

Improvements to the tex rendering pipeline #362

Merged
merged 10 commits into from
Jun 14, 2020

Conversation

JonasIsensee
Copy link
Contributor

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

  • There are no changes to JMarkdown2tex

  • TexMinted and Tex now have a template field and by default use the md2pdf.tpl template

  • The template was modified to allow for additional \usepackage{...} dependencies (defined in docformat.tex_deps)

  • TexMinted now produces runnable tex files but because Highlights.jl only works with lstlisting the standard code highlighting is deactivated.

  • Tex does not produce a runnable tex file because there is currently no code to define the custom code environments.

@aviatesk
Copy link
Member

aviatesk commented Jun 3, 2020

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.

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left few feedbacks, but in general this is really nice enhancements !

@@ -185,7 +198,7 @@ function format_chunk(chunk::DocChunk, docformat::JMarkdown2tex)
return docformat.keep_unicode ? out : uc2tex(out)
end

function format_output(result, docformat::JMarkdown2tex)
function format_output(result, docformat::TexFormat)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will produce "invalid" output texminted format, right ?
Hightlights.jl will include its own characters around some special characters e.g. _, so that it will result in weird output for minted output.
How about defining new format_output for TexMinted ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted. I hadn't figured this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed it.
However it did make my code have more duplicate lines again.

Comment on lines 166 to 168
highlight_str(docformat::TexFormat) = ""
highlight_str(docformat::JMarkdown2tex) =
get_highlight_stylesheet(MIME("text/latex"), docformat.highlight_theme)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe defining render_doc for JMarkdown2tex and TexFormat separately will make more sense ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is probably true. This saved a handful of lines but of course it needs another new functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defined render_doc separately now

@aviatesk
Copy link
Member

aviatesk commented Jun 7, 2020

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

@JonasIsensee
Copy link
Contributor Author

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
Copy link
Member

aviatesk commented Jun 13, 2020

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
Copy link
Contributor Author

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.

register_format!("md2tex", JMarkdown2tex())
register_format!("md2pdf", JMarkdown2tex())

@aviatesk
Copy link
Member

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

@JonasIsensee
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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.

Comment on lines 220 to 229
function uc2tex(::TexFormat, s, escape = false)
for key in keys(latex_symbols)
if escape
s = replace(s, latex_symbols[key] => "|\$\\ensuremath{$(texify(key))}\$|")
else
s = replace(s, latex_symbols[key] => "\\ensuremath{$(texify(key))}")
end
end
return s
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like:

uc2tex(::JMarkdown2tex, s, escape = false) = unicode2tex(s, escape, "*@")
uc2tex(::TexFormat, s, escape = false) = unicode2tex(s, escape, "|\$")
function unicode2tex(s, escape, starter, closer = reverse(starter))
    for key in keys(latex_symbols)
        body = "\\ensuremath{$(texify(key))}"
        target = escape ? string(starter, body, closer) : body
        s = replace(s, latex_symbols[key] => body)
    end
    return s
end

would look more cleaner ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, that's much nicer !
Please put that instead 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or actually I don't think we can just define unicode2tex and don't use dispatch here; it only adds complexity here imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I don't quite get what you are trying to say.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ups, sorry, very bad english, I tied to say:
I think we can just define unicode2tex and remove uc2tex. Using dispatches doesn't seem to be necessary here imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I'm starting to understand.
The dispatch is indeed not necessary, but on the other hand, uc2tex is currently called 5 times and in one of those cases (

return docformat.keep_unicode ? out : uc2tex(docformat, out)
)

you can't hardcode the escape sequence. (Because the calling scope is used for all <:TexFormats.

There is however no real need for two names:

unicode2tex(::JMarkdown2tex, s, escape = false) = unicode2tex(s, escape, "*@")
unicode2tex(::TexMinted,     s, escape = false) = unicode2tex(s, escape, "|\$"))

function unicode2tex(s, escape, starter, closer = reverse(starter))
    for key in keys(latex_symbols)
        body = "\\ensuremath{$(texify(key))}"
        target = escape ? string(starter, body, closer) : body
        s = replace(s, latex_symbols[key] => body)
    end
    return s
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively escape and the escape sequence starter and closer could be made part of the JMarkdown2tex and TexMinted struct

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds fair, I will like whichever way you would like to choose.

@aviatesk
Copy link
Member

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

@JonasIsensee
Copy link
Contributor Author

oops, I broke the tests again.

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2020

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%) ⬇️
Impacted Files Coverage Δ
src/rendering/rendering.jl 88.88% <ø> (ø)
src/rendering/texformats.jl 66.31% <57.14%> (-22.19%) ⬇️
src/Weave.jl 46.00% <100.00%> (+0.54%) ⬆️
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.

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for your great work, @JonasIsensee !

@aviatesk aviatesk merged commit 174585e into JunoLab:master Jun 14, 2020
@aviatesk aviatesk added breaking change for records PDF PDF output tex about TeX/LaTeX documents labels Jun 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for records PDF PDF output tex about TeX/LaTeX documents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants