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

Allow switching of -no-shell-escape flag #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bomb-on
Copy link
Contributor

@bomb-on bomb-on commented Jun 24, 2016

Passing allow_shell_escape=True to build_pdf() would switch the default -no-shell-escape flag into -shell-escape.

Passing `allow_shell_escape=True` to `build_pdf()` would switch the default `-no-shell-escape` flag into `-shell-escape`.
@mbr
Copy link
Owner

mbr commented Feb 19, 2017

If this is still current, I do not think this is the right way to go about it -- I think it'd be better to derive a custom builder, like InsecurePDFLatexBuilder (if you wish, you can substitute "Trusted" for "Insecure" ;)).

The reason for this is that not every conceivable Builder necessarily has shell escapes. Correct me if I'm wrong there!

@tom-f-oconnell
Copy link

This issue is still relevant. I personally don't think there's much more than aesthetic difference between the solution you're describing and the one in the pull request, and I like the minimal extra code in the pull request's solution.

It doesn't seem like such a problem that builders not using this flag would just silently ignore the keyword argument. Additionally, such builders are also only hypothetical right now, as all of the builders currently implemented do have these shell escape flags. People who implement additional builders could err if there are unrecognized keyword arguments, if they so choose.

Since we already have this fix, absent someone willing to implement it as you envision, I would appreciate this being merged. Until then, I'll just depend on a fork with this patch in the mean time.

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.

4 participants