barrysteyn / pelican_plugin-render_math

A plugin for the pelican blog to allow it render mathematics in Latex.
67 stars 25 forks source link

Commit 4e790ddc0410e0986103869a66a69fd1e99afb8b breaks plugin #13

Closed hydrogen18 closed 10 years ago

hydrogen18 commented 10 years ago

Your latest commit in the plugin breaks it for me. After reverting it, it works without a problem.

It blows up on math.py line 151. You are hiding most of exception on that line, which makes it very difficult to identify the underlying problem. From what I can tell it blows up with KeyError for 'math_tag_class' somewhere in the markdown package. I don't know enough about markdown & your plugin to fix it. I recommend calling sys.excepthook(*sys.exc_info()) to display the exception to the user before failing.

I'm running on Ubuntu 14.04 LTS Python Stackless 2.7.8 Markdown 2.5 pelican - latest from github

barrysteyn commented 10 years ago

Hi Eric

I'll see what is wrong and fix it, but I'm not able to do it today. I'll try for tomorrow On Sep 20, 2014 11:43 AM, "Eric Urban" notifications@github.com wrote:

Your latest commit in the plugin breaks it for me. After reverting it, it works without a problem.

It blows up on math.py line 151. You are hiding most of exception on that line, which makes it very difficult to identify the underlying problem. From what I can tell it blows up with KeyError for 'math_tag_class' somewhere in the markdown package. I don't know enough about markdown & your plugin to fix it. I recommend calling sys.excepthook(*sys.exc_info()) to display the exception to the user before failing.

I'm running on Ubuntu 14.04 LTS Python Stackless 2.7.8 Markdown 2.5 pelican - latest from github

— Reply to this email directly or view it on GitHub https://github.com/barrysteyn/pelican_plugin-render_math/issues/13.

lukewarm commented 10 years ago

I think it has something to do with the changes incorporated to Python-Markdown v2.5. Reverting to Python-Markdown v2.4.1 gets rid of the problem, for now. I noticed this because another pelican plugin (liquid_tag) that extends Markdown has the same KeyError, which is set off by the Extension.setConfig in Python Markdown 2.5.

I think this error would occur after the changes to init.py from this commit of the Markdown package,

The comment seems to suggest default values must be given to config.

class Extension(object):
    """ Base class for extensions to subclass. """

    # Default config -- to be overriden by a subclass
    # Must be of the following format:
    #     {
    #       'key': ['value', 'description']
    #     }
    # Note that Extension.setConfig will raise a KeyError
    # if a default is not set here.

`

barrysteyn commented 10 years ago

Hi

I will be back from vacation on Sun 29th, and I will look at it then. Right now, mountains in Banff :)

On Sat, Sep 20, 2014 at 1:02 PM, lukewarm notifications@github.com wrote:

I think it has something to do with the changes incorporated to Python-Markdown v2.5. Reverting to Python-Markdown v2.4.1 gets rid of the problem, for now. I noticed this because another pelican plugin (liquid_tag) that extends Markdown has the same KeyError, which is set off by the Extension.setConfig https://github.com/waylan/Python-Markdown/blob/master/markdown/extensions/__init__.py#L64 in Python Markdown 2.5.

I think this error would occur after the changes to init.py from this commit of the Markdown package https://github.com/waylan/Python-Markdown/commit/47372051cf9724f1355b1c07c63c4beff9a5f626,

The comment https://github.com/waylan/Python-Markdown/blob/master/markdown/extensions/__init__.py#L13 seems to suggest default values must be given to config.

class Extension(object): """ Base class for extensions to subclass. """

# Default config -- to be overriden by a subclass
# Must be of the following format:
#     {
#       'key': ['value', 'description']
#     }
# Note that Extension.setConfig will raise a KeyError
# if a default is not set here.

`

— Reply to this email directly or view it on GitHub https://github.com/barrysteyn/pelican_plugin-render_math/issues/13#issuecomment-56278701 .

hydrogen18 commented 10 years ago

@lukewarm You nailed it. I fixed it, although there might be a more robust refactor in order.

barrysteyn commented 10 years ago

Hi Eric

When I get back, I'll refractor the code. Can you tell me what is the issue? On Sep 21, 2014 2:09 PM, "Eric Urban" notifications@github.com wrote:

@lukewarm https://github.com/lukewarm You nailed it. I fixed it, although there might be a more robust refactor in order.

— Reply to this email directly or view it on GitHub https://github.com/barrysteyn/pelican_plugin-render_math/issues/13#issuecomment-56310912 .

hydrogen18 commented 10 years ago

The issue is that you must set default values for all configuration keys in the config dictionary of a pelican markdown extension before using super to call the parent class constructor. I just set the empty string as the default for the mathjax script and used the existing value for math_tag_class as the default.

Also fixed

barrysteyn commented 10 years ago

It looks good, thanks. There is just one or Terry l two things I want to change, for instance, I don't want to hard code anything within the extension class itself (this is because I'm getting the plugin ready for katex) and other things. But this is a big help and I'll make sure the changes are updated this evening.

Thanks so much Barry On Sep 22, 2014 12:14 PM, "Eric Urban" notifications@github.com wrote:

The issue is that you must set default values for all configuration keys in the config dictionary of a pelican markdown extension before using super to call the parent class constructor. I just set the empty string as the default for the mathjax script and used the existing value for math_tag_class as the default.

Also fixed

  • When the markdown extension class constructor is called keyword args should be passed so I used **config. This avoids a deprecation warning.
  • Your class that adds the mathjax script to the document was returning None when it should return the root node.
    • The getConfig call was returning a list of length one so I just added [0] to the code. Not sure if this is Markdown 2.5 or something else I introduced.

— Reply to this email directly or view it on GitHub https://github.com/barrysteyn/pelican_plugin-render_math/issues/13#issuecomment-56415400 .

barrysteyn commented 10 years ago

Yikes,I keep on forgetting I'm on vacation :) I'll probably only get to doing this by Sun. But thank you so much for the effort. With regards to an array being returned, that is strange. I must look into it when I get time.

Barry On Sep 22, 2014 5:15 PM, "Barry Steyn" barry.steyn@gmail.com wrote:

It looks good, thanks. There is just one or Terry l two things I want to change, for instance, I don't want to hard code anything within the extension class itself (this is because I'm getting the plugin ready for katex) and other things. But this is a big help and I'll make sure the changes are updated this evening.

Thanks so much Barry On Sep 22, 2014 12:14 PM, "Eric Urban" notifications@github.com wrote:

The issue is that you must set default values for all configuration keys in the config dictionary of a pelican markdown extension before using super to call the parent class constructor. I just set the empty string as the default for the mathjax script and used the existing value for math_tag_class as the default.

Also fixed

  • When the markdown extension class constructor is called keyword args should be passed so I used **config. This avoids a deprecation warning.
  • Your class that adds the mathjax script to the document was returning None when it should return the root node.
    • The getConfig call was returning a list of length one so I just added [0] to the code. Not sure if this is Markdown 2.5 or something else I introduced.

— Reply to this email directly or view it on GitHub https://github.com/barrysteyn/pelican_plugin-render_math/issues/13#issuecomment-56415400 .

hydrogen18 commented 10 years ago

The array being returned was my own bug. I corrected it and updated the pull request.

barrysteyn commented 10 years ago

Hi

Okay, so unfortunately, markdown 2.5 adds non-backward compatible requirements. So I have pushed a version of the plugin that will work for all versions. Can you please test it

hydrogen18 commented 10 years ago

I just tried it using Markdown 2.5. It works.