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

Liquid tags #21

Merged
merged 24 commits into from Aug 28, 2013

Conversation

Projects
None yet
@jakevdp
Contributor

jakevdp commented May 3, 2013

This adds a set of extensions to use Liquid-style tags within markdown (similar to those provided by octopress). There are four tags included at this time, though the module is written to be easily extensible:

  • Insert images of a given size:

    {% img /url/to/image.png [width] [height] [title] [alt]%}
    
  • Insert HTML5/flash compatible videos:

    {% video /url/to/video.mp4 [width] [height] [title] %}
    
  • Insert code from a file, with a title and link to download:

    {% include_code filename [title] %}
    
  • Insert an HTML-rendered ipython notebook:

    {% notebook filename.ipynb [cells[start:end]] %}
    

Examples and details of how to use each are in the Readme file, and in the doc strings of each plugin.

@jakevdp jakevdp referenced this pull request May 6, 2013

Merged

add header extra args #13

@almet

This comment has been minimized.

Show comment
Hide comment
@almet

almet May 17, 2013

Member

that looks great.

However, I would love to have this not only bound to markdown, but also available for people using restructured text, do you think that would be useful?

Member

almet commented May 17, 2013

that looks great.

However, I would love to have this not only bound to markdown, but also available for people using restructured text, do you think that would be useful?

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp May 18, 2013

Contributor

I agree that would be nice, but would basically require writing and testing an entirely new implementation. The current one is built on markdown preprocessors, and you'd have to do the equivalent with ReST preprocessors as well. I've never dug into ReST - do you know if it would be straightforward to do?

Contributor

jakevdp commented May 18, 2013

I agree that would be nice, but would basically require writing and testing an entirely new implementation. The current one is built on markdown preprocessors, and you'd have to do the equivalent with ReST preprocessors as well. I've never dug into ReST - do you know if it would be straightforward to do?

@mlgill

This comment has been minimized.

Show comment
Hide comment
@mlgill

mlgill May 24, 2013

When I attempted to used the Liquid tags notebook plugin from this pull request (21) with a very simple markdown post and IPython notebook, I get the following error upon running pelican:

WARNING: Could not process /Users/mlgill/Dropbox/themodernscientist/content/2013-05-24-testing_ipython_notebook.md
parsing error: didn't find the end of the div

If I comment out lines 144-145 of notebook.py and rerun pelican, everything is fine and the html post looks as expected. Thus, I believe the error is not valid. I have only briefly looked into debugging the issue and haven't yet figured out the problem.

Here is some information about my setup:

* IPython version 0.13.2
* Liquid tags from pull request 21 of pelican-plugins
* nbconvert github commit 0fababc961cbe0b58df4cf0286e6b2c41fb695c53
* nbconvert requires `style.min.css` to be placed in `IPython/frontend/html/notebook/static/css` and IPython version 0.13.2 doesn't have this file, so I've tried this using `style.min.css` from github commit c751c59af3ed736332cf6e2f61d27e39ccc5a788. However, the issue persists when using `notebook.css` from IPython version 0.13.2 instead.
* Pelican version 3.2.1
* Python version 2.7.5
* Mac OS X version 10.8.3

Please let me know if I can provide further information or test something.

mlgill commented May 24, 2013

When I attempted to used the Liquid tags notebook plugin from this pull request (21) with a very simple markdown post and IPython notebook, I get the following error upon running pelican:

WARNING: Could not process /Users/mlgill/Dropbox/themodernscientist/content/2013-05-24-testing_ipython_notebook.md
parsing error: didn't find the end of the div

If I comment out lines 144-145 of notebook.py and rerun pelican, everything is fine and the html post looks as expected. Thus, I believe the error is not valid. I have only briefly looked into debugging the issue and haven't yet figured out the problem.

Here is some information about my setup:

* IPython version 0.13.2
* Liquid tags from pull request 21 of pelican-plugins
* nbconvert github commit 0fababc961cbe0b58df4cf0286e6b2c41fb695c53
* nbconvert requires `style.min.css` to be placed in `IPython/frontend/html/notebook/static/css` and IPython version 0.13.2 doesn't have this file, so I've tried this using `style.min.css` from github commit c751c59af3ed736332cf6e2f61d27e39ccc5a788. However, the issue persists when using `notebook.css` from IPython version 0.13.2 instead.
* Pelican version 3.2.1
* Python version 2.7.5
* Mac OS X version 10.8.3

Please let me know if I can provide further information or test something.

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp May 24, 2013

Contributor

That piece is a bit of an ugly hack -- once the nbconvert architecture is stabilized, we should be able to select cells within nbconvert, rather than slicing up the generated html.

It would be better to try and fix this once the nbconvert refactor is finished.

Contributor

jakevdp commented May 24, 2013

That piece is a bit of an ugly hack -- once the nbconvert architecture is stabilized, we should be able to select cells within nbconvert, rather than slicing up the generated html.

It would be better to try and fix this once the nbconvert refactor is finished.

@mlgill

This comment has been minimized.

Show comment
Hide comment
@mlgill

mlgill May 25, 2013

No problem. I knew what this code was doing and this is why I didn't spend tons of time debugging it since I could myself hack it and make things work. Given the situation with nbconvert, I figured you'd want to wait. At least it's documented now as having been an issue for the future.

mlgill commented May 25, 2013

No problem. I knew what this code was doing and this is why I didn't spend tons of time debugging it since I could myself hack it and make things work. Given the situation with nbconvert, I figured you'd want to wait. At least it's documented now as having been an issue for the future.

@ibayer

This comment has been minimized.

Show comment
Hide comment
@ibayer

ibayer Jun 13, 2013

Hey, I'm quit interested in this plug in. Do I need to expect significant changes before this PR gets merged?
@jakevdp Thanks for your work!

ibayer commented Jun 13, 2013

Hey, I'm quit interested in this plug in. Do I need to expect significant changes before this PR gets merged?
@jakevdp Thanks for your work!

@justinmayer

This comment has been minimized.

Show comment
Hide comment
@justinmayer

justinmayer Jun 13, 2013

Member

Thanks for the contribution, Jake. To recap, here's where we are so far:

@ametaireau said:

I would love to have this not only bound to Markdown, but also available for people using reStructuredText, do you think that would be useful?

@jakevdp replied:

I agree that would be nice, but would basically require writing and testing an entirely new implementation. The current one is built on Markdown preprocessors, and you'd have to do the equivalent with reST preprocessors as well. I've never dug into reST - do you know if it would be straightforward to do?

I agree reST support would be nice, but I suspect we should merge this in its current form for the following reasons:

  1. It's a plugin (not core) and thus perhaps can be afforded more "leeway"
  2. Implementing support for reST may be a non-trivial task
  3. Nobody has stepped forward to implement equivalent support for reST

Last but not least, even if we merge this plugin in its current form, anybody is of course free to pick this up again in the future to implement support for reST, Asciidoc, and other formats.

Just my two cents. Any other thoughts?

Member

justinmayer commented Jun 13, 2013

Thanks for the contribution, Jake. To recap, here's where we are so far:

@ametaireau said:

I would love to have this not only bound to Markdown, but also available for people using reStructuredText, do you think that would be useful?

@jakevdp replied:

I agree that would be nice, but would basically require writing and testing an entirely new implementation. The current one is built on Markdown preprocessors, and you'd have to do the equivalent with reST preprocessors as well. I've never dug into reST - do you know if it would be straightforward to do?

I agree reST support would be nice, but I suspect we should merge this in its current form for the following reasons:

  1. It's a plugin (not core) and thus perhaps can be afforded more "leeway"
  2. Implementing support for reST may be a non-trivial task
  3. Nobody has stepped forward to implement equivalent support for reST

Last but not least, even if we merge this plugin in its current form, anybody is of course free to pick this up again in the future to implement support for reST, Asciidoc, and other formats.

Just my two cents. Any other thoughts?

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Jun 13, 2013

Contributor

Actually, I'd like to first make sure the notebook piece works with the finished nbconvert refactor -- I haven't had a chance to do that piece yet. While we're at it, we may as well wait until nbconvert is merged into IPython for the 1.0 release, slated for July.

Contributor

jakevdp commented Jun 13, 2013

Actually, I'd like to first make sure the notebook piece works with the finished nbconvert refactor -- I haven't had a chance to do that piece yet. While we're at it, we may as well wait until nbconvert is merged into IPython for the 1.0 release, slated for July.

@almet

This comment has been minimized.

Show comment
Hide comment
@almet

almet Jun 28, 2013

Member

Any update?

Member

almet commented Jun 28, 2013

Any update?

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Jun 28, 2013

Contributor

Not yet - I'm still planning to wait for the IPython 1.0 release, otherwise we'll have to rewrite this code again in a month.

Contributor

jakevdp commented Jun 28, 2013

Not yet - I'm still planning to wait for the IPython 1.0 release, otherwise we'll have to rewrite this code again in a month.

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Jul 3, 2013

Contributor

Just so folks can keep up-to-date -- I've been working on getting the features into IPython 1.0 that will allow this PR to be finished, and finished well. IPython work here:

Contributor

jakevdp commented Jul 3, 2013

Just so folks can keep up-to-date -- I've been working on getting the features into IPython 1.0 that will allow this PR to be finished, and finished well. IPython work here:

@mlgill

This comment has been minimized.

Show comment
Hide comment
@mlgill

mlgill Jul 3, 2013

@jakevdp Thanks for the info. Have been waiting patiently for all the necessary updates. Happy to give it a try when ready.

mlgill commented Jul 3, 2013

@jakevdp Thanks for the info. Have been waiting patiently for all the necessary updates. Happy to give it a try when ready.

@ibayer

This comment has been minimized.

Show comment
Hide comment
@ibayer

ibayer Jul 3, 2013

@jakevdp thanks for the update. I'm eager to give it a try too.

ibayer commented Jul 3, 2013

@jakevdp thanks for the update. I'm eager to give it a try too.

@almet

This comment has been minimized.

Show comment
Hide comment
@almet

almet Jul 11, 2013

Member

Awesome, we're almost there!

Member

almet commented Jul 11, 2013

Awesome, we're almost there!

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Jul 16, 2013

Contributor

This commit reworks the notebook tag to use the new IPython.nbconvert submodule, which is part of the yet-to-be-released IPython 1.0. This requires a dev install of IPython (two relevant PRs were merged into IPython master earlier today).

As long as nothing else changes in IPython before the 1.0 release, I'd say this is good to go. There may be a few adjustments to the IPython stylesheets before that, though, so I'd prefer to wait until IPython 1.0 is finalized before merging this.

In the meantime, if anyone here is willing to test and give feedback on the new code, I'd appreciate it!

Contributor

jakevdp commented Jul 16, 2013

This commit reworks the notebook tag to use the new IPython.nbconvert submodule, which is part of the yet-to-be-released IPython 1.0. This requires a dev install of IPython (two relevant PRs were merged into IPython master earlier today).

As long as nothing else changes in IPython before the 1.0 release, I'd say this is good to go. There may be a few adjustments to the IPython stylesheets before that, though, so I'd prefer to wait until IPython 1.0 is finalized before merging this.

In the meantime, if anyone here is willing to test and give feedback on the new code, I'd appreciate it!

Show outdated Hide outdated liquid_tags/notebook.py
end = Integer(MAX_NB_CELLS, config=True,
help="last cell of notebook to be converted")
def __call__(self, nb, resources):

This comment has been minimized.

@Carreau

Carreau Jul 17, 2013

you can just overwrite call (without dunder) it is just a stupid wrapper in NBconvert for now, but we might add conveniences stuff later (like being sure to pass a deepcopy of nb instead of a reference).

I think for 1.0 all the "Activatable" prefix will be dropped and the base class will support the enable keyword (just FYI).

@Carreau

Carreau Jul 17, 2013

you can just overwrite call (without dunder) it is just a stupid wrapper in NBconvert for now, but we might add conveniences stuff later (like being sure to pass a deepcopy of nb instead of a reference).

I think for 1.0 all the "Activatable" prefix will be dropped and the base class will support the enable keyword (just FYI).

Show outdated Hide outdated liquid_tags/notebook.py
for css_line in resources['inlining']['css'])
header += JS_INCLUDE
open('_nb_header.html', 'w').write(header)

This comment has been minimized.

@Carreau

Carreau Jul 17, 2013

does this open (and the one line 253) close the file ? (maybe python does automatically if the file descriptor has no more references, i'm just not sure)

@Carreau

Carreau Jul 17, 2013

does this open (and the one line 253) close the file ? (maybe python does automatically if the file descriptor has no more references, i'm just not sure)

This comment has been minimized.

@jakevdp

jakevdp Jul 17, 2013

Contributor

It should, but it's probably better to use a context manager in any case. I'll change that.

@jakevdp

jakevdp Jul 17, 2013

Contributor

It should, but it's probably better to use a context manager in any case. I'll change that.

Show outdated Hide outdated liquid_tags/notebook.py
if end:
end = int(end)
else:
end = MAX_NB_CELLS

This comment has been minimized.

@Carreau

Carreau Jul 17, 2013

I think that slicing with None should work and mean 'end':

In [9]: range(10)[5:None]
Out[9]: [5, 6, 7, 8, 9]
@Carreau

Carreau Jul 17, 2013

I think that slicing with None should work and mean 'end':

In [9]: range(10)[5:None]
Out[9]: [5, 6, 7, 8, 9]

This comment has been minimized.

@jakevdp

jakevdp Jul 17, 2013

Contributor

True, but doesn't the IPython Integer traitlet crash on None?

@jakevdp

jakevdp Jul 17, 2013

Contributor

True, but doesn't the IPython Integer traitlet crash on None?

This comment has been minimized.

@Carreau

Carreau Jul 17, 2013

Hum, that is true... most of Traitlets accept None, but, Not Integer. I'll see if this could be added on IPython side. like IntegerMaybeNone.

@Carreau

Carreau Jul 17, 2013

Hum, that is true... most of Traitlets accept None, but, Not Integer. I'll see if this could be added on IPython side. like IntegerMaybeNone.

This comment has been minimized.

@jakevdp

jakevdp Jul 17, 2013

Contributor

That would be useful. I suppose I could also define a custom traitlet for the purpose of this plugin if you don't think it's general enough to include in IPython. Let me know what you decide.

@jakevdp

jakevdp Jul 17, 2013

Contributor

That would be useful. I suppose I could also define a custom traitlet for the purpose of this plugin if you don't think it's general enough to include in IPython. Let me know what you decide.

SYNTAX = '{% img [class name(s)] [http[s]:/]/path/to/image [width [height]] [title text | "title text" ["alt text"]] %}'
# Regular expression to match the entire syntax
ReImg = re.compile("""(?P<class>\S.*\s+)?(?P<src>(?:https?:\/\/|\/|\S+\/)\S+)(?:\s+(?P<width>\d+))?(?:\s+(?P<height>\d+))?(?P<title>\s+.+)?""")

This comment has been minimized.

@almet

almet Jul 17, 2013

Member

In general, put the constants in full upper case

@almet

almet Jul 17, 2013

Member

In general, put the constants in full upper case

ReTitleAlt = re.compile("""(?:"|')(?P<title>[^"']+)?(?:"|')\s+(?:"|')(?P<alt>[^"']+)?(?:"|')""")
@LiquidTags.register('img')

This comment has been minimized.

@almet

almet Jul 17, 2013

Member

I love the use of the decorator for that.

@almet

almet Jul 17, 2013

Member

I love the use of the decorator for that.

@almet

This comment has been minimized.

Show comment
Hide comment
@almet

almet Jul 17, 2013

Member

i'm re-reading the whole issue. @jakevdp I think adding the same thing for restructured text wouldn't be hard.

Member

almet commented Jul 17, 2013

i'm re-reading the whole issue. @jakevdp I think adding the same thing for restructured text wouldn't be hard.

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Jul 18, 2013

Contributor

I think I addressed all the notebook.py comments.
@Carreau - I ended up defining a custom traitlet that allows None for the slicing. It's only a few lines, and I think it makes things much cleaner. Thanks for the suggestion!

Contributor

jakevdp commented Jul 18, 2013

I think I addressed all the notebook.py comments.
@Carreau - I ended up defining a custom traitlet that allows None for the slicing. It's only a few lines, and I think it makes things much cleaner. Thanks for the suggestion!

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Jul 18, 2013

I think I addressed all the notebook.py comments.

@Carreau - I ended up defining a custom traitlet that allows None for the slicing. It's only a few lines, and I think it makes things much cleaner. Thanks for the suggestion!

Yes, looks great.

I think that since a few hours ago s/ActivatableTransformers/Transformers/g I'll re-read the plugin (or at least the part that I understand)

Carreau commented Jul 18, 2013

I think I addressed all the notebook.py comments.

@Carreau - I ended up defining a custom traitlet that allows None for the slicing. It's only a few lines, and I think it makes things much cleaner. Thanks for the suggestion!

Yes, looks great.

I think that since a few hours ago s/ActivatableTransformers/Transformers/g I'll re-read the plugin (or at least the part that I understand)

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Jul 18, 2013

Contributor

The last commit keeps this compatible with the current IPython master.

Contributor

jakevdp commented Jul 18, 2013

The last commit keeps this compatible with the current IPython master.

@ulikoehler

This comment has been minimized.

Show comment
Hide comment
@ulikoehler

ulikoehler Jul 23, 2013

Contributor

+1, I'd also like to see this merged as soon as possible, reST functionality can be added later, but to me, this plugin provides a significant usage improvement, especially when including ipython notebooks.

Contributor

ulikoehler commented Jul 23, 2013

+1, I'd also like to see this merged as soon as possible, reST functionality can be added later, but to me, this plugin provides a significant usage improvement, especially when including ipython notebooks.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Jul 23, 2013

We are sprinting on nbconvert this week, so I will sugest waiting for 1.0 release.
We kkep you in mind :-)

Carreau commented Jul 23, 2013

We are sprinting on nbconvert this week, so I will sugest waiting for 1.0 release.
We kkep you in mind :-)

@ulikoehler

This comment has been minimized.

Show comment
Hide comment
@ulikoehler

ulikoehler Jul 23, 2013

Contributor

Wow, thanks for the quick answer - I'm happy to hear that and looking forward to the results :-)

Contributor

ulikoehler commented Jul 23, 2013

Wow, thanks for the quick answer - I'm happy to hear that and looking forward to the results :-)

help="last cell of notebook to be converted")
def call(self, nb, resources):
nbc = deepcopy(nb)

This comment has been minimized.

@Carreau

Carreau Jul 23, 2013

Will now take care of that for you :-)

@Carreau

Carreau Jul 23, 2013

Will now take care of that for you :-)

This comment has been minimized.

@Carreau

Carreau Aug 7, 2013

nband resources are now guaranted to be deepcopied before beeing passed to the first transformer, so you shoudl be safe of not deepcopying it yourself if you are concern with performance.

@Carreau

Carreau Aug 7, 2013

nband resources are now guaranted to be deepcopied before beeing passed to the first transformer, so you shoudl be safe of not deepcopying it yourself if you are concern with performance.

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Aug 6, 2013

Contributor

For anyone curious, this currently does not work with the IPython release candidate (some things have been renamed). I'm working on remedying that.

Contributor

jakevdp commented Aug 6, 2013

For anyone curious, this currently does not work with the IPython release candidate (some things have been renamed). I'm working on remedying that.

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Aug 7, 2013

Contributor

Turns out is was a simple name remapping. This should work with the IPython 1.0 release candidate available at this tag: https://github.com/ipython/ipython/tree/1.0.0a1

There are a few nbconvert bug fixes in the process of being backported from master for release. Again, we should wait until IPython 1.0 is final to merge, but I think this is pretty close!

Contributor

jakevdp commented Aug 7, 2013

Turns out is was a simple name remapping. This should work with the IPython 1.0 release candidate available at this tag: https://github.com/ipython/ipython/tree/1.0.0a1

There are a few nbconvert bug fixes in the process of being backported from master for release. Again, we should wait until IPython 1.0 is final to merge, but I think this is pretty close!

@justinmayer

This comment has been minimized.

Show comment
Hide comment
@justinmayer

justinmayer Aug 7, 2013

Member

Fantastic, Jake. Excited for the release!

Member

justinmayer commented Aug 7, 2013

Fantastic, Jake. Excited for the release!

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Aug 7, 2013

Oups, sorry for the name conflict, I try to warn you when there are changes that might affect the plugin, but this one slipped through my fingers.

Carreau commented Aug 7, 2013

Oups, sorry for the name conflict, I try to warn you when there are changes that might affect the plugin, but this one slipped through my fingers.

- First, the plugin requires that the nbconvert package [1]_ to be in the
python path. For example, in bash, this can be set via
>$ export PYTHONPATH=/path/to/nbconvert/

This comment has been minimized.

@pelson

pelson Aug 10, 2013

Think this needs updating since nbconvert is now in IPython.

@pelson

pelson Aug 10, 2013

Think this needs updating since nbconvert is now in IPython.

Because the notebook relies on some rather extensive custom CSS, the use of
this plugin requires additional CSS to be inserted into the blog theme.
After typing "make html" when using the notebook tag, a file called
``_nb_header.html`` will be produced in the main directory. The content

This comment has been minimized.

@pelson

pelson Aug 10, 2013

Sadly the _nb_header.html didn't quite cover all of the bases for color selection and I ended up having to add .highlight-ipynb .n { color: black; } manually to the header to see certain sections of my code. I'm using the Just-Read theme...

@pelson

pelson Aug 10, 2013

Sadly the _nb_header.html didn't quite cover all of the bases for color selection and I ended up having to add .highlight-ipynb .n { color: black; } manually to the header to see certain sections of my code. I'm using the Just-Read theme...

This comment has been minimized.

@jakevdp

jakevdp Aug 15, 2013

Contributor

Is this because the default text color is non-black?

@jakevdp

jakevdp Aug 15, 2013

Contributor

Is this because the default text color is non-black?

This comment has been minimized.

@pelson

pelson Aug 16, 2013

I guess so (It's inherited from the template). What it essentially means is that the css being generated by the pygments call is not sufficient to cover the html which it is generating, which suggests there is a synchronisation issue. I've tried to dig into it, but tbh, its all pretty deep inside IPython and then even deeper in pygments.

@pelson

pelson Aug 16, 2013

I guess so (It's inherited from the template). What it essentially means is that the css being generated by the pygments call is not sufficient to cover the html which it is generating, which suggests there is a synchronisation issue. I've tried to dig into it, but tbh, its all pretty deep inside IPython and then even deeper in pygments.

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Aug 15, 2013

Contributor

We'll need to watch out for this change, though it's IPython 2.0: ipython/ipython#4044

Contributor

jakevdp commented Aug 15, 2013

We'll need to watch out for this change, though it's IPython 2.0: ipython/ipython#4044

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Aug 16, 2013

We will probably also rename Exporter -> TemplateExporter as we will have one more base classe.

Carreau commented Aug 16, 2013

We will probably also rename Exporter -> TemplateExporter as we will have one more base classe.

# Below is the pelican plugin code.
#
SYNTAX = "{% notebook /path/to/notebook.ipynb [ cells[start:end] ] %}"
FORMAT = re.compile(r"""^(\s+)?(?P<src>\S+)(\s+)?((cells\[)(?P<start>-?[0-9]*):(?P<end>-?[0-9]*)(\]))?(\s+)?$""")

This comment has been minimized.

@pelson

pelson Aug 17, 2013

It'd be nice if this could handle spaces in the filename. Perhaps allowing people to specify the filename in quotes would disambiguate the situation?

@pelson

pelson Aug 17, 2013

It'd be nice if this could handle spaces in the filename. Perhaps allowing people to specify the filename in quotes would disambiguate the situation?

This comment has been minimized.

@jakevdp

jakevdp Aug 20, 2013

Contributor

I agree that would be nice, but I think adding this sort of thing would overly-complicate an already extremely dense regular expression. Something like this would work:

FORMAT = re.compile(r"""^(\s+)?(?P<src>\"[^\"]*\"|\'[^\']*\'|\S+)(\s+)?((cells\[)(?P<start>-?[0-9]*):(?P<end>-?[0-9]*)(\]))?(\s+)?$""")

But then you end up with a string containing quotation marks if they're used, which adds further complicated post-processing. Additionally, a regular expression like this is extremely difficult to read, maintain, and understand, even if you're the person who wrote it!

The best option would be to write a simple modular parsing framework to express these things within the submodule, but I don't really have the bandwidth to take that on right now. Plus, who puts spaces in their filenames anymore? 😄

@jakevdp

jakevdp Aug 20, 2013

Contributor

I agree that would be nice, but I think adding this sort of thing would overly-complicate an already extremely dense regular expression. Something like this would work:

FORMAT = re.compile(r"""^(\s+)?(?P<src>\"[^\"]*\"|\'[^\']*\'|\S+)(\s+)?((cells\[)(?P<start>-?[0-9]*):(?P<end>-?[0-9]*)(\]))?(\s+)?$""")

But then you end up with a string containing quotation marks if they're used, which adds further complicated post-processing. Additionally, a regular expression like this is extremely difficult to read, maintain, and understand, even if you're the person who wrote it!

The best option would be to write a simple modular parsing framework to express these things within the submodule, but I don't really have the bandwidth to take that on right now. Plus, who puts spaces in their filenames anymore? 😄

@gvwilson

This comment has been minimized.

Show comment
Hide comment
@gvwilson

gvwilson Aug 17, 2013

This looks cool - will you be adding {% for %} and {% if %}, and if so, can they wrap cells, i.e., can I put:

+------------------------------+
| {% if instructor %} |
+------------------------------+
| ...a regular Python cell... |
+------------------------------+
| ...a Markdown cell... |
+------------------------------+
| {% endif %} |
+------------------------------+

and have it work as expected? Because I'd use that right away :-)

This looks cool - will you be adding {% for %} and {% if %}, and if so, can they wrap cells, i.e., can I put:

+------------------------------+
| {% if instructor %} |
+------------------------------+
| ...a regular Python cell... |
+------------------------------+
| ...a Markdown cell... |
+------------------------------+
| {% endif %} |
+------------------------------+

and have it work as expected? Because I'd use that right away :-)

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Aug 18, 2013

Contributor

@gvwilson - that's a bit beyond the scope of this. The liquid tags are within the pelican markdown file, not within the notebook itself, so I'm not sure how that would work in this context.

Contributor

jakevdp commented Aug 18, 2013

@gvwilson - that's a bit beyond the scope of this. The liquid tags are within the pelican markdown file, not within the notebook itself, so I'm not sure how that would work in this context.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Aug 18, 2013

@gvwilson this would go into nbconvert template/transformers, then in pelican you could do :

{% notebook /path/to/notebook.ipynb [ instructor | learner ] %}

Carreau commented Aug 18, 2013

@gvwilson this would go into nbconvert template/transformers, then in pelican you could do :

{% notebook /path/to/notebook.ipynb [ instructor | learner ] %}

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Aug 20, 2013

Contributor

Aside from several feature requests which I likely won't get to, I think this is ready to merge.
I've been using it with IPython 1.0 on my blog for about a week now, and all the bugs seem to be worked out.

There's certainly going to be work in the future to make it compatible with IPython 2.0, and perhaps add a better parsing framework than the raw regular expressions I'm currently using. Also, adding RST support would be nice, if someone wants to take that on.

None of those are blockers, though, and I'd like to finalize what we have.

Contributor

jakevdp commented Aug 20, 2013

Aside from several feature requests which I likely won't get to, I think this is ready to merge.
I've been using it with IPython 1.0 on my blog for about a week now, and all the bugs seem to be worked out.

There's certainly going to be work in the future to make it compatible with IPython 2.0, and perhaps add a better parsing framework than the raw regular expressions I'm currently using. Also, adding RST support would be nice, if someone wants to take that on.

None of those are blockers, though, and I'd like to finalize what we have.

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Aug 26, 2013

Contributor

A user noted that if sphinx is not installed, then the IPython version check was giving a misleading message. I fixed that.

Contributor

jakevdp commented Aug 26, 2013

A user noted that if sphinx is not installed, then the IPython version check was giving a misleading message. I fixed that.

@AlexMikhalev

This comment has been minimized.

Show comment
Hide comment
@AlexMikhalev

AlexMikhalev Aug 27, 2013

What is the current status of this pull request?

What is the current status of this pull request?

@jakevdp

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Aug 27, 2013

Contributor

Ready for merge

Contributor

jakevdp commented Aug 27, 2013

Ready for merge

@justinmayer

This comment has been minimized.

Show comment
Hide comment
@justinmayer

justinmayer Aug 28, 2013

Member

Thanks to Jake for all the hard work on this plugin, with due appreciation to others who assisted.

Member

justinmayer commented Aug 28, 2013

Thanks to Jake for all the hard work on this plugin, with due appreciation to others who assisted.

justinmayer added a commit that referenced this pull request Aug 28, 2013

@justinmayer justinmayer merged commit a20ca76 into getpelican:master Aug 28, 2013

@ocefpaf

This comment has been minimized.

Show comment
Hide comment
@ocefpaf

ocefpaf Sep 9, 2013

I believe that this comparison should be with a string ('1') instead of an integer (1).

I believe that this comparison should be with a string ('1') instead of an integer (1).

This comment has been minimized.

Show comment
Hide comment
@jakevdp

jakevdp Sep 9, 2013

Owner

Yes, this was fixed here: 1bda804

Owner

jakevdp replied Sep 9, 2013

Yes, this was fixed here: 1bda804

This comment has been minimized.

Show comment
Hide comment

Thanks.

@jakevdp jakevdp deleted the jakevdp:liquid_tags branch Jun 7, 2018

@almet

This comment has been minimized.

Show comment
Hide comment
@almet

almet Jun 8, 2018

Member
Member

almet commented Jun 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment