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

Separate writing from filter application in toJSONFilters #49

Merged
merged 2 commits into from
Nov 5, 2016

Conversation

mpacer
Copy link
Contributor

@mpacer mpacer commented Nov 5, 2016

This separates the current toJSONFilters functionality into two parts:

  • toJSONFilters (which maintains the previous API, writes to stdout, reads from stdin, and extracts command line arguments)
  • applyJSONFilters (which works on JSON strings, iterating through and applying the filters).

This is needed to be able to use JSONfilters written for pandoc within an already existing python process (related to finding a solution to jupyter/nbconvert#11 with a particular application to be found at jupyter/nbconvert#458).

With help from @takluyver & inspiration from https://github.com/takluyver/bookbook.

@takluyver
Copy link

To expand a bit, the reason we're doing this is that we're invoking pandoc from Python, and we've found performance is significantly better if we use pandoc twice (to json, from json) and apply the filter in process than if we run pandoc and get it to run the filter in a Python subprocess. This is probably because starting a Python process has a relatively large overhead.

We still want to write the filters in a compatible way, so we adapted the code a little bit so that we can apply them to the output.

@jgm jgm merged commit a8a6303 into jgm:master Nov 5, 2016
@jgm
Copy link
Owner

jgm commented Nov 5, 2016

Sensible!

@takluyver
Copy link

Thanks John! I see we just missed a release a couple of weeks ago - any idea when this is likely to go into a release so we can rely on it?

@jgm
Copy link
Owner

jgm commented Nov 5, 2016

I made some docstring updates, bumpde the version,
and updated README. Can you look it over and try it out?
If it all looks okay I can publish another release.

+++ Thomas Kluyver [Nov 05 16 12:04 ]:

Thanks John! I see we just missed a release a couple of weeks ago - any
idea when this is likely to go into a release so we can rely on it?


You are receiving this because you modified the open/close state.
Reply to this email directly, [1]view it on GitHub, or [2]mute the
thread.

References

  1. Separate writing from filter application in toJSONFilters #49 (comment)
  2. https://github.com/notifications/unsubscribe-auth/AAAL5Ir9on_Wz2vo-FghxuUidVufq8o3ks5q7NM0gaJpZM4KqG6X

@takluyver
Copy link

It looks good to me. I think @michaelpacer successfully ran our machinery with the code he added, but I'll let him confirm that.

@mpacer
Copy link
Contributor Author

mpacer commented Nov 8, 2016

Apologies for the delay; yes, this seems to work on my end just fine 👍

@jgm
Copy link
Owner

jgm commented Nov 9, 2016

I've uploaded the release.

+++ Michael Pacer [Nov 08 16 14:56 ]:

Apologies for the delay; yes, this seems to work on my end just fine 👍


You are receiving this because you modified the open/close state.
Reply to this email directly, [1]view it on GitHub, or [2]mute the
thread.

References

  1. Separate writing from filter application in toJSONFilters #49 (comment)
  2. https://github.com/notifications/unsubscribe-auth/AAAL5Cin5y_q6k1iIzujsZedOGKG0ASjks5q8P4LgaJpZM4KqG6X

@takluyver
Copy link

Thanks @jgm !

@mpacer
Copy link
Contributor Author

mpacer commented Nov 10, 2016

Thank you @jgm!

On Wed, Nov 9, 2016 at 3:07 PM, Thomas Kluyver notifications@github.com
wrote:

Thanks @jgm https://github.com/jgm !


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#49 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACXg6IcYyX7LIbM3rjEZL-NVSJyxBVHPks5q8lIVgaJpZM4KqG6X
.

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.

3 participants