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

Minted filter for beamer and latex, safe for all writers #40

Merged
merged 8 commits into from
Jan 17, 2019
Merged

Minted filter for beamer and latex, safe for all writers #40

merged 8 commits into from
Jan 17, 2019

Conversation

svenevs
Copy link
Member

@svenevs svenevs commented Jan 10, 2019

Hey! I made a new years resolution to get this all nice and shiny and in lua, and I think it's looking pretty good so far.

  • initial filter and documentation
  • finish addressing issues / suggestions in tarleb's review!

Copy link
Member Author

@svenevs svenevs left a comment

Choose a reason for hiding this comment

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

These are the things I'm a little concerned about, but overall I think this works pretty well!

If you would rather not have such a complicated filter that's OK too. But I think this would be a really helpful filter for anybody who wants to change the listings for beamer / tex 🙂

minted/minted.lua Show resolved Hide resolved
minted/minted.lua Outdated Show resolved Hide resolved
minted/minted.lua Outdated Show resolved Hide resolved
minted/minted.lua Outdated Show resolved Hide resolved
minted/minted.lua Show resolved Hide resolved
@jgm
Copy link
Member

jgm commented Jan 10, 2019 via email

jgm added a commit to jgm/pandoc that referenced this pull request Jan 10, 2019
@svenevs
Copy link
Member Author

svenevs commented Jan 11, 2019

Ok the proposed final documentation is rendered here. I just need to add some tests later tonight and then it should be good (I'll add a new comment and remove [WIP] from title when ready for final review).

When we think this is ready for merge, is it ok if I squash these all into one commit? The commit messages aren't very good, and the diff stack is unnecessary (I had some back and forth ...). I hesitate to do this because when I do I think GitHub removes the review comments with the questions I have above. But if they aren't actually problems, then I'll force push the badness away :)

@tarleb
Copy link
Member

tarleb commented Jan 11, 2019

Great! Feel free to squash, but we can also squash this when merging (we generally do this for all PRs introducing new filters).

@svenevs
Copy link
Member Author

svenevs commented Jan 12, 2019

ok i think this looks good 🙂

i hope the tests being written in python is ok, i had to do a lot of string manipulation and that's definitely my go-to language for that. grepping for the latex was annoying because latex syntax definitely conflicts with regex syntax.

sorry for multiple force pushes :D but now it's just one commit, unless things need to change in which case future commits will be structured better and can just be squash merged on your end.

@svenevs svenevs changed the title [WIP] Minted filter for beamer and latex Minted filter for beamer and latex Jan 12, 2019
Copy link
Member

@tarleb tarleb left a comment

Choose a reason for hiding this comment

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

❤️

minted/minted.lua Show resolved Hide resolved
minted/minted.lua Outdated Show resolved Hide resolved
minted/minted.lua Outdated Show resolved Hide resolved
minted/minted.lua Outdated Show resolved Hide resolved
minted/minted.lua Outdated Show resolved Hide resolved
minted/minted.lua Outdated Show resolved Hide resolved
minted/minted.lua Outdated Show resolved Hide resolved
minted/minted.lua Show resolved Hide resolved
- elem.attributes preferred to elem.attr[3]
- use `kind` as parameter name, overloading `type`
  makes debugging more difficult
update tests to make sure that `fragile` is not
present in non-beamer output
make sure tests assert that `nil` is not present to ensure
that metadata keys are not being processed incorrectly
@svenevs svenevs changed the title Minted filter for beamer and latex Minted filter for beamer and latex, safe for all writers Jan 17, 2019
@svenevs
Copy link
Member Author

svenevs commented Jan 17, 2019

Ok round two complete, thanks for the really helpful review on making this much cleaner @tarleb! All of your feedback has been incorporated and a few updates to the tests to actually validate the Header (ensure_not_present(test_name, "fragile", output)) in the non-beamer stuff, and make sure metadata doesn't produce nil. I think this looks really good now 🙂

IIRC when squashed the title of the PR is what the main commit message is. I don't know what the main message should be, but feel free to edit it to be whatever you want.

@tarleb tarleb merged commit a078b61 into pandoc:master Jan 17, 2019
@tarleb
Copy link
Member

tarleb commented Jan 17, 2019

Thanks again! This is probably the best documented and tested filter in the collection.

@svenevs svenevs deleted the minted branch January 17, 2019 19:22
@svenevs
Copy link
Member Author

svenevs commented Jan 17, 2019

Woohoo, you're very welcome! Hehe yes ... I got a little carried a way xD

@mfhepp
Copy link

mfhepp commented Aug 18, 2022

Am impressed by how well this filter is written and documented! Dedication to detail at its best :-)!

I hope this is the best place to leave this info for others: For the ArXiv preprints workflow, there are some limitations on using minted. In essence, it is important to use a non-hidden cache directory, like so

\usepackage[frozencache=true,cachedir=minted-cache]{minted}

The important thing is that the cachedir name does not start with a dot as the default .minted-cache.

Full details are here: https://arxiv.org/help/faq/mistakes#minted

Disclaimer: I did not test this yet, but thought it is worth logging it for the future.

@svenevs @tarleb

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.

None yet

4 participants