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

render_math: Fix for new MARKDOWN dict in Pelican 3.7 #787

Merged
merged 1 commit into from
Jan 22, 2017
Merged

render_math: Fix for new MARKDOWN dict in Pelican 3.7 #787

merged 1 commit into from
Jan 22, 2017

Conversation

Krastanov
Copy link
Contributor

This extensions does not work with the current git version of pelican. This fix hopefully addresses the issue (works for me).

@justinmayer
Copy link
Member

Thanks for the fix, Stefan. I will most likely merge getpelican/pelican#1927 soon so it can be included in the upcoming Pelican 3.7 release, at which point you will probably be asked to update this pull request. Apologies for the moving target! (^_^)

@Krastanov
Copy link
Contributor Author

Thanks for the heads up! Should I understand that the two repos are considered to be tightly coupled and there is no need for backwards compatibility with pelican 3.6.3?

@justinmayer
Copy link
Member

Backwards compatibility is always preferred but not strictly necessary.

Now that Pelican 3.7 has been released, would you be so kind as to update this pull request to use the new MARKDOWN setting?

@justinmayer justinmayer changed the title Fix for MD_EXTENSIONS becoming a dict in 3.6.4 Fix for new MARKDOWN dict in 3.7 Dec 13, 2016
@Krastanov
Copy link
Contributor Author

Will do in a bit.

@justinmayer
Copy link
Member

Hi Stefan. Any updates on this?

@Krastanov
Copy link
Contributor Author

@justinmayer I made the change. It might not work on 3.6.4, but it should work on the current version and on versions before 3.6.4. I made the change somewhat blindly (I do not have understanding of what those options do, I just followed the changes described in the pull request that removed MD_EXTENSIONS). I did test it and it seems to work for me.

if isinstance(pelicanobj.settings.get('MD_EXTENSIONS'), list): # pelican 3.6.3 and earlier
pelicanobj.settings['MD_EXTENSIONS'].append(PelicanMathJaxExtension(config))
else:
pelicanobj.settings['MARKDOWN']['extension_configs'].update({PelicanMathJaxExtension(config):{}})
Copy link
Member

@Scheirle Scheirle Dec 17, 2016

Choose a reason for hiding this comment

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

Class instance extensions should be added to the extensions list, extension_configs is only used for extensions loaded by name. See http://pythonhosted.org/Markdown/reference.html#markdown

pelicanobj.settings['MARKDOWN'].setdefault('extensions', []).append(PelicanMathJaxExtension(config))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, that was embarrassing. I will make that change.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Krastanov yes please 👍

Copy link

@cranmer cranmer Dec 26, 2016

Choose a reason for hiding this comment

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

So I tried both versions of the patch above and it isn't working. I think I'm having some conflict with the pelican-ipynb.liquid plugin (some mathjax conflict?) because if I comment out the line

pelicanobj.settings['MARKDOWN'].setdefault('extensions', []).append(PelicanMathJaxExtension(config))```

then the latex is rendered properly. I'm running pelican-3.7.0

@jar1karp
Copy link
Contributor

While you're at it, please fix the process_summary function also. At the moment I'm getting this warning:

WARNING: _get_summary() has been deprecated since 3.6.4. Use the summary decorator instead

and Mathjax is not loaded even if a summary contains math in index pages.

@Krastanov
Copy link
Contributor Author

@jar1karp I will try to track this down.

Copy link
Contributor

@Natim Natim left a comment

Choose a reason for hiding this comment

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

I feel like we need that for more plugins actually. liquid_tags too for instance.

if isinstance(pelicanobj.settings.get('MD_EXTENSIONS'), list): # pelican 3.6.3 and earlier
pelicanobj.settings['MD_EXTENSIONS'].append(PelicanMathJaxExtension(config))
else:
pelicanobj.settings['MARKDOWN']['extension_configs'].update({PelicanMathJaxExtension(config):{}})
Copy link
Contributor

Choose a reason for hiding this comment

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

@Krastanov yes please 👍

@Natim
Copy link
Contributor

Natim commented Dec 22, 2016

  • better_codeblock_line_numbering/
  • extract_toc/
  • headerid/
  • liquid_tags/
  • md_inline_extension/
  • render_math/

Maybe we should add a shared helper that plugins can use for that.

@jar1karp
Copy link
Contributor

Re: the problem with auto-generated summaries -- it seems that the render_math plugin can't update the summary anymore as the get_summary method is memoized (changed in this commit). The plugin gets a truncated summary (i.e. the Mathjax code also gets truncated), does the cleanup and reinserts the Mathjax, but it doesn't actually change the summary. The html will be rendered using the truncated Mathjax so math will not display correctly.

The plugin seems to work correctly if the memoized decorator is commented out for get_summary in contents.py. But I suppose it needs a cleaner fix, maybe a proper mechanism for plugins to be allowed to update summaries?

In addition, if using summary plugin with render_math they have to be loaded in that order to work correctly (render_math would otherwise strip away the special summary marker comments).

@danmackinlay
Copy link
Contributor

danmackinlay commented Jan 3, 2017

I broke the summary issue out into a new issue since it seems to be orthogonal to the basic markdown fix - although in fact there already exists one. See #784.

@danmackinlay
Copy link
Contributor

The alternative pull request referenced above is a minor change - it adds the required math markdown extension instnce directly to the extensions list rather than adding it to the extensions_config list with an empty config. AFAICT from the Markdown docs , this is preferred by Markdown. I believe this change is cosmetic?

@Krastanov
Copy link
Contributor Author

Krastanov commented Jan 4, 2017

In (hopefully) conclusion:

@justinmayer
Copy link
Member

Thanks to Stefan for the fix and to others for reviewing and assisting. Much obliged! (^_^)

@justinmayer justinmayer changed the title Fix for new MARKDOWN dict in 3.7 render_math: Fix for new MARKDOWN dict in 3.7 Jan 22, 2017
@justinmayer justinmayer changed the title render_math: Fix for new MARKDOWN dict in 3.7 render_math: Fix for new MARKDOWN dict in Pelican 3.7 Jan 22, 2017
@justinmayer justinmayer merged commit b726362 into getpelican:master Jan 22, 2017
@szhorvat
Copy link
Contributor

Is this a separately maintained version of render_math in getpelican/pelican-plugins? I didn't realize there were several versions. Here's the same issue in the original repo:

barrysteyn/pelican_plugin-render_math#39

@jerryasher
Copy link

Regarding @szhorvat's comment, I do wonder, what is the recommended practice when finding/fixing bugs in various Pelican plugins?

I submitted a patch to this bug at the author's repo.
Should I also have submitted a patch (it would have been different as the modules have diverged a bit) over here as well?

(I mean, I guess so, but I didn't want to step on any toes...)

@drorata
Copy link
Contributor

drorata commented Mar 16, 2017

When executing ./develop_server.sh start I still get a warning

WARNING: _get_summary() has been deprecated since 3.6.4. Use the summary decorator instead

which, as far as I understand, relates to this thread. I fetched all changes to the plugins. The sources in my case can be found here. How can I avoid this warning?

gondree added a commit to gondree/pelicanfly that referenced this pull request Jun 15, 2017
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.

10 participants