Reworking format.jl #350

JonasIsensee · 2020-05-24T12:17:09Z

Hi,
addressing #345
This PR implements a super simple way to make exports for tex and texminted work.
I'm not saying that this is the best way to do it but I just started this PR as a proof of concept.

It may be worth reorganizing the types around the docformats to account for the similarities (such as shown here).

aviatesk · 2020-05-24T12:32:42Z

nice, thanks for your try.
so can this PR produce "valid" tex files for tex and texminted formats, as is ?

If so, I'm totally okay to merge this, while it would be super great if you could tell any problem you notice.

It may be worth reorganizing the types around the docformats to account for the similarities (such as shown here).

sounds good, might be good to start in another PR, tho ;)

JonasIsensee · 2020-05-24T19:51:58Z

nice, thanks for your try.
so can this PR produce "valid" tex files for tex and texminted formats, as is ?

If so, I'm totally okay to merge this, while it would be super great if you could tell any problem you notice.

Yeah, in my initial tests this did seem to produce a valid .tex file.
But I'll check again to make sure.

EDIT: actually no. texminted needs, for obvious reasons, the latex package minted which would have to be added in the template .tpl file.
However currently there is no good way to include a different template file for texminted by default..

Also, given that the tests now fail it seems that the old behaviour was intended, though it's not clear what for.

aviatesk · 2020-05-25T00:52:42Z

great,

However currently there is no good way to include a different template file for texminted by default..

so the loading of minted is the only modifcation that should be added to our existing template ? If so I have an idea to reuse the existing template in a way we add minted only when we weave to texminted format.

Also, given that the tests now fail it seems that the old behaviour was intended, though it's not clear what for.

yeah we have tons of old and very fragile (and annoying tbh) end2end tests, and I guess this PR can fail some of them, but don't care it. We can manually update them later (or even remove them). see #326 for the context if you want.

aviatesk · 2020-05-25T03:17:33Z

fwiw, rebasing against master will make it easier to run tests locally (skip Plots.jl and Gadfly.jl integration tests, which would take long time to finish otherwise)

JonasIsensee · 2020-05-25T08:57:49Z

so the loading of minted is the only modifcation that should be added to our existing template ? If so I have an idea to reuse the existing template in a way we add minted only when we weave to texminted format.

Yeah, I also thought about that.
Right now the md2pdf.tpl loads the listings package. One could either copy the file and edit it or alternatively find a way to dynamically add the corresponding line. On the other hand this does not scale well, if we end up needing to edit more lines...

Maybe it would make sense to make each .formatdict for the various output formats contain a path to the default template.

get_template(::Nothing, tex::Bool = false) =
    Mustache.template_from_file(normpath(TEMPLATE_DIR, tex ? "md2pdf.tpl" : "md2html.tpl"))

that would allow us to remove this hardly extendable piece of code.

well, we can do much more things using this, but this commit just introduces this type Co-Authored-By: null <[email protected]>

- and load minted package for TexMinted format Co-Authored-By: null <[email protected]>

aviatesk · 2020-05-25T11:41:31Z

alternatively find a way to dynamically add the corresponding line.

yep, we can do that, see this README section in Mustache.jl, a template engine we use.

I pushed some commits, and now texminted::TexMinted generates .tex files which loads minted, ... but I found they're not really valid tex files.
In other word, they can't be compiled, since now format_hoge (e.g. format_chunk) produces some tex string, but all of them assume that they use listing package. So the next thing we need to do is to overload those functions with TexMinted format to produce tex strings, which can be compiled when joined.

aviatesk · 2020-05-30T13:28:17Z

okay, I updated this PR to #355 .
Sorry if the previous rebasing makes you unable to push/pull this PR.
Deleting your local branch and pulling again from your repository will make it work. I will make sure I merge master rather than rebasing in the future ;)

JonasIsensee · 2020-05-30T15:36:10Z

No worries.

So I'm restructuring the files right now and moving code back and forth.

I really don't like the fact that we have these weird types that are all the same because they just contain a dictionary. This seems like an unnecessary bit of indirection.

Since no part of Weave.jl is performance critical (and since the current approach isn't exactly type stable either) I propose that we do away with the dictionary in between and define untyped
structs with many fields leveraging Base.@kwdef for instantiation.

Somewhere (probably at the very beginning) we would then have to @assert that the types have all the fields that are needed but then everywhere else in the code it would make lookups simpler.
docformat.formatdict[:property] → docformat.property

EDIT: all these asserts are only necessary for user passed types. But this is not the intended usage anyway, since then she would need to implement all the methods anyway? The more likely/intended case is that users pass a modified instance of one of the implemented types.
Any then Base.@kwdef does asserts along with defaults for us.

@aviatesk if you disagree, please say so. If not, I will start working on that tomorrow.

… directly

JonasIsensee · 2020-05-31T08:22:56Z

My thoughts on the following lines of code. (at Weave.jl line 210 ish)
This is really un-julian style.

doctype = doc.doctype
    if doctype == "pandoc2html"
        mdname = outname
        outname = get_outname(out_path, doc, ext = "html")
        pandoc2html(rendered, doc, highlight_theme, outname, pandoc_options)
        rm(mdname)
    elseif doctype == "pandoc2pdf"
        mdname = outname
        outname = get_outname(out_path, doc, ext = "pdf")
        pandoc2pdf(rendered, doc, outname, pandoc_options)
        rm(mdname)
    elseif doctype == "md2pdf"
        run_latex(doc, outname, latex_cmd)
        outname = get_outname(out_path, doc, ext = "pdf")
    end

I think we could instead implement a postprocessing function that generally does nothing
but runs the external commands for the formats for which something else is defined.

post_processing!(::WeaveFormat) = nothing
post_processing!(::Pandoc2HTML) = stuff

JonasIsensee · 2020-05-31T09:25:22Z

Oh, I found a proper issue that will make working with minted and tex harder..

https://github.com/JuliaDocs/Highlights.jl/blob/fc1d9494f06a3ec9db923cda7c9369b63cc96e44/src/Format.jl#L121

Highlights.jl works with lstlisting and only with that.
If we want to make texminted available at all we will have to disable Highlight themes with that.

JonasIsensee · 2020-05-31T09:53:25Z

ok, so I'm finally starting to understand what is happening here:
In md2tex:

code and term chunks are highlighted using Highlights.jl. This package automatically wraps the highlighted code in lstlistings environments. (and does not seem to be able to use minted)
This is why in JMarkdown2tex codestart == codeend == ""
output is not highlighted but only wrapped in docformat.outputstart , docformat.outputend

This means that if we want to make texminted and tex output valid tex files we have to skip the highlight part. That is definitely a downside but without implementing new highlighting themes for Highlights.jl this seems to be the only option.

JonasIsensee · 2020-05-31T11:27:24Z

By now a lot of files have changed.
Here's a quick summary:

Reworked the types. <: WeaveFormats don't contain a dict anymore but instead a lot of untyped fields. If you don't like that we can go back to dicts but keep the new access pattern by overloading getproperty(df::WeaveFormat, symbol) = df.symbol
all formats currently weave without errors but since I don't know what all the formats are meant to do I can't say that they do what they are supposed to do
texminted compiles without errors but due to missing highlighting (see above) it is not as pretty
Tex outputs a decent looking tex file but without a proper template that defines the code environments it won't compile
md2tex & md2pdf produce pretty pdfs

JonasIsensee · 2020-05-31T11:31:50Z

Things that remain to be done:

Implement better argument parsing to properly support the new structure
use that to pass around fewer arguments in function signatures
Make argument parsing test for sensible values (?)
make preserving header a field of the format instead of
const HEADER_PRESERVE_DOCTYPES = ("github", "hugo")
code could do with more polishing

Things like https://github.com/mauro3/Unpack.jl could also shorten the code in some places.

aviatesk · 2020-05-31T15:00:11Z

Thanks for your work on this, again. Most of your suggestions sound so nice.

Before discussing details, I would like to ask if you would mind splitting this PR into separate PRs, one for general refactoring format/rendering logic, one for actually improving Tex output format ?
That way, we can see if your refactoring keeps backward compats or breaks something, etc. Having said that, we don't necessary keep backward compats, and I would really want to your refactoring to get in. It will just make it easier to review/merge easily.

aviatesk · 2020-05-31T15:10:53Z

Here are comments on refactoring parts:

EDIT: all these asserts are only necessary for user passed types. But this is not the intended usage anyway, since then she would need to implement all the methods anyway? The more likely/intended case is that users pass a modified instance of one of the implemented types.
Any then Base.@kwdef does asserts along with defaults for us.

I like it. Actually I had the exact same idea before (maybe a week ago) but at that time I withdraw it for some reason. I forgot the detail already but I guess at that time we did something like merge!(formatdict, something) in somewhere, but now I can't find it anywhere and I also don't think such a merging and modifying instantiated formats are necessary, so let's go that way.

I think we could instead implement a postprocessing function that generally does nothing
but runs the external commands for the formats for which something else is defined.

sounds nice.

Implement better argument parsing to properly support the new structure

yeah, we can do smarter processing of rendering-related arguments/options according to output formats.

make preserving header a field of the format instead of const HEADER_PRESERVE_DOCTYPES = ("github", "hugo")

yeah, sounds good.

JonasIsensee · 2020-05-31T15:11:04Z

hm, that is a very good point.
I'm not super sure on how to best split this stuff into different PRs though..
I'll give it a try.

JonasIsensee · 2020-05-31T16:08:54Z

So I created a new PR but since the branch of this PR lives in my own fork of Weave,
the new PR lives in my fork entirely...
This is not what we wanted, no?

JonasIsensee#1

- reorder/move things so that format-specific rendering methods are defined close to the format - separate common rendering methods from format specific methods - fix using location

- remove unused field defs - in theory extensible, once documented - fix test - start to change terminology, `format` -> `render`

aviatesk · 2020-06-01T13:40:41Z

So I created a new PR but since the branch of this PR lives in my own fork of Weave,
the new PR lives in my fork entirely...
This is not what we wanted, no?

ah, sorry, that is only possible if you can push a brach into this repository, so let's do tex stuff in another PR after this PR gets merged into master.

aviatesk · 2020-06-01T13:44:44Z

I pushed two commits into this PR since I can't make another PR merging into this PR: each does:

code style preference
- reorder/move things so that format-specific rendering methods are defined close to each format
- separate common rendering methods from format specific methods
- fix using location
set rendering options with dispatch, which enables us:
- remove unused field defs
- in theory extensible, once documented
- fix test
- start to change terminology, format -> render

So can you kindly review on this ?

aviatesk · 2020-06-01T13:49:49Z

src/rendering/htmlformats.jl

+Base.@kwdef mutable struct JMarkdown2HTML <: HTMLFormat
+    description = "Julia markdown to html"
+    codestart = "\n"
+    codeend = "\n"
+    outputstart = "<pre class=\"output\">"
+    outputend = "</pre>\n"
+    fig_ext = ".png"
+    mimetypes = ["image/png", "image/jpg", "image/svg+xml",
+                "text/html", "text/markdown", "text/plain"]
+    extension = "html"
+    termstart = codestart
+    termend = codeend
+    out_width = nothing
+    out_height = nothing
+    fig_pos = nothing
+    fig_env = nothing
+    template = nothing
+    stylesheet = nothing
+    highlight_theme = nothing
+end


Maybe we can do even more that this, like creating a macro that wraps Base.@kwdef and automatically defines some fields like mimetypes with default value if not specified, and do some assertion, etc, but we can do that later.

That's actually not too bad an idea! I like it

aviatesk · 2020-06-01T13:51:31Z

And, imho, this kind of refactoring PR should be merged soon as possible (otherwise we may start to feel really dull on this kind of stuff), so I think I will move to quickly merge this if you approve and we pass tests.

JonasIsensee · 2020-06-02T05:59:04Z

Ah, you started outsourcing the argument handling already. Nice!

I like your changes. I think we can merge.

- first, it's really dull to keep compatibility with LTS - and now we want to use `Base.@kwdef` for creating types with supertypes, and LTS can't have that for reasons

aviatesk · 2020-06-02T06:49:58Z

@JonasIsensee okay, I will move to merge this once CI passes.
We've dropped LTS compatibility support, but who complains.

Looking forward to your another tex improvements PR on top of this ! (well, take your time, as always ;))

JonasIsensee and others added 3 commits May 25, 2020 20:29

introduce common supertype for JMarkdown2tex and Tex

e200428

introduce WeaveFormat abstract type

2fdd09b

well, we can do much more things using this, but this commit just introduces this type Co-Authored-By: null <[email protected]>

define render_doc and dispatch on it with different formats

f4ed10b

- and load minted package for TexMinted format Co-Authored-By: null <[email protected]>

aviatesk force-pushed the texexports branch from 3eb2a59 to f4ed10b Compare May 25, 2020 11:37

Merge branch 'master' into texexports

e7637de

fix typing and improve @define_format

db89118

JonasIsensee added 2 commits May 30, 2020 17:49

move around code into separate files

119b333

removing duplicate code

6e99822

JonasIsensee changed the title ~~[WIP] Introduce common supertype for JMarkdown2tex and Tex~~ [WIP] Reworking formatters May 30, 2020

JonasIsensee added 2 commits May 31, 2020 09:29

remove format dict - make everything a field of format <: WeaveFormat…

770496b

… directly

fix lots of things I missed before

62d5684

make texminted output valid texfile

3eba5de

JonasIsensee added 2 commits May 31, 2020 12:07

some cleanup

45adb89

pass fewer arguments and bugfixes

9875bc1

rename files and include location + move texify

543f99a

aviatesk added 2 commits June 1, 2020 21:32

refactoring:

ae0dc70

- reorder/move things so that format-specific rendering methods are defined close to the format - separate common rendering methods from format specific methods - fix using location

set rendering options with dispatch:

55d2c97

- remove unused field defs - in theory extensible, once documented - fix test - start to change terminology, `format` -> `render`

Merge branch 'master' into texexports

0bdab33

aviatesk reviewed Jun 1, 2020

View reviewed changes

aviatesk changed the title ~~[WIP] Reworking formatters~~ Reworking format.jl Jun 1, 2020

aviatesk removed PDF PDF output tex about TeX/LaTeX documents labels Jun 1, 2020

aviatesk force-pushed the texexports branch 2 times, most recently from 8c86974 to b294856 Compare June 1, 2020 15:03

aviatesk closed this Jun 2, 2020

aviatesk reopened this Jun 2, 2020

aviatesk force-pushed the texexports branch from b294856 to 0267d64 Compare June 2, 2020 01:32

aviatesk added 5 commits June 2, 2020 15:31

reorder WeaveFormat field defs

19b4a79

use type field to restore header check

ae48729

drop LTS support:

c6b23d4

- first, it's really dull to keep compatibility with LTS - and now we want to use `Base.@kwdef` for creating types with supertypes, and LTS can't have that for reasons

reorder functions again

4b12f3d

a bit more reorder

04b24ea

aviatesk force-pushed the texexports branch from 0267d64 to 04b24ea Compare June 2, 2020 06:46

bump up to julia 1.2

0760591

aviatesk added 2 commits June 2, 2020 19:00

use isnothing & isempty

aa7fd31

type annotate API

533667b

aviatesk approved these changes Jun 2, 2020

View reviewed changes

aviatesk merged commit 73bf7fe into JunoLab:master Jun 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reworking format.jl #350

Reworking format.jl #350

JonasIsensee commented May 24, 2020

aviatesk commented May 24, 2020 •

edited

Loading

JonasIsensee commented May 24, 2020 •

edited

Loading

aviatesk commented May 25, 2020 •

edited

Loading

aviatesk commented May 25, 2020

JonasIsensee commented May 25, 2020 •

edited

Loading

aviatesk commented May 25, 2020

aviatesk commented May 30, 2020

JonasIsensee commented May 30, 2020 •

edited

Loading

JonasIsensee commented May 31, 2020

JonasIsensee commented May 31, 2020 •

edited

Loading

JonasIsensee commented May 31, 2020

JonasIsensee commented May 31, 2020

JonasIsensee commented May 31, 2020 •

edited

Loading

aviatesk commented May 31, 2020

aviatesk commented May 31, 2020

JonasIsensee commented May 31, 2020

JonasIsensee commented May 31, 2020 •

edited

Loading

aviatesk commented Jun 1, 2020 •

edited

Loading

aviatesk commented Jun 1, 2020 •

edited

Loading

aviatesk Jun 1, 2020

JonasIsensee Jun 2, 2020

aviatesk commented Jun 1, 2020 •

edited

Loading

JonasIsensee commented Jun 2, 2020

aviatesk commented Jun 2, 2020 •

edited

Loading

Reworking format.jl #350

Reworking format.jl #350

Conversation

JonasIsensee commented May 24, 2020

aviatesk commented May 24, 2020 • edited Loading

JonasIsensee commented May 24, 2020 • edited Loading

aviatesk commented May 25, 2020 • edited Loading

aviatesk commented May 25, 2020

JonasIsensee commented May 25, 2020 • edited Loading

aviatesk commented May 25, 2020

aviatesk commented May 30, 2020

JonasIsensee commented May 30, 2020 • edited Loading

JonasIsensee commented May 31, 2020

JonasIsensee commented May 31, 2020 • edited Loading

JonasIsensee commented May 31, 2020

JonasIsensee commented May 31, 2020

JonasIsensee commented May 31, 2020 • edited Loading

aviatesk commented May 31, 2020

aviatesk commented May 31, 2020

JonasIsensee commented May 31, 2020

JonasIsensee commented May 31, 2020 • edited Loading

aviatesk commented Jun 1, 2020 • edited Loading

aviatesk commented Jun 1, 2020 • edited Loading

aviatesk Jun 1, 2020

Choose a reason for hiding this comment

JonasIsensee Jun 2, 2020

Choose a reason for hiding this comment

aviatesk commented Jun 1, 2020 • edited Loading

JonasIsensee commented Jun 2, 2020

aviatesk commented Jun 2, 2020 • edited Loading

aviatesk commented May 24, 2020 •

edited

Loading

JonasIsensee commented May 24, 2020 •

edited

Loading

aviatesk commented May 25, 2020 •

edited

Loading

JonasIsensee commented May 25, 2020 •

edited

Loading

JonasIsensee commented May 30, 2020 •

edited

Loading

JonasIsensee commented May 31, 2020 •

edited

Loading

JonasIsensee commented May 31, 2020 •

edited

Loading

JonasIsensee commented May 31, 2020 •

edited

Loading

aviatesk commented Jun 1, 2020 •

edited

Loading

aviatesk commented Jun 1, 2020 •

edited

Loading

aviatesk commented Jun 1, 2020 •

edited

Loading

aviatesk commented Jun 2, 2020 •

edited

Loading