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

Unifying compiler extensions #1864

Closed
wants to merge 7 commits into from
Closed

Conversation

@felixfontein
Copy link
Contributor

felixfontein commented Jul 5, 2015

In c65be6e Kwpolska added a new plugin class, CompilerExtension, and made MarkdownExtension and RestExtension subclasses of this new plugin class. This PR moves the plugin loading code from the Rest and Markdown plugins into a common place in PageCompiler, which can be used by all page compiler plugins.

Review on Reviewable

felixfontein added 2 commits Jul 4, 2015
…iding simple accessing functions for compiler extensions for a given page compiler.
…et_compiler_extensions().
@felixfontein felixfontein added this to the v7.6.1 milestone Jul 5, 2015
@Kwpolska
Copy link
Member

Kwpolska commented Jul 5, 2015

Broken: https://ci.appveyor.com/project/Kwpolska/nikola/build/1.0.1865#L1681
Yapsy probably does not think of them as CompilerExtensions, it looks like we have to activate all three names in nikola/nikola.py.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

Yapsy does find them. There's another problem, and I'm currently working on it.

…ion and MarkdownExtension class definitions as plugins and ignore the real plugin defined in the same file.
@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

We have to activate all three names, but not because yapsy won't identify derived classes of RestExtension and MarkdownExtension as CompilerExtension plugins, but because yapsy would identify the imported RestExtension or MarkdownExtension class definition as a plugin and would often ignore the real plugin defined in the file.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

Part of the problem is I think that many plugins in Nikola ignore the advice here: http://yapsy.sourceforge.net/Advices.html#plugin-class-detection-caveat

@Kwpolska
Copy link
Member

Kwpolska commented Jul 5, 2015

We can’t fix it now, there would be so many things to change.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

I assume that the failed invariance test is part of another PR. The rest seems to work fine now.

@@ -239,5 +242,17 @@ def __init__(self):
self.template_system = self
self.name = 'mako'

def _activate_plugins_of_category(self, category):

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Jul 5, 2015

Member

This is code duplication, is it possible to import nikola.nikola and do FakeSite._activate_plugins_of_category = nikola.nikola.Nikola._activate_plugins_of_category instead?

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jul 5, 2015

Author Contributor

Seems to work locally for me, now let's see what Travis and AppVeyor say :)

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jul 5, 2015

Author Contributor

Doesn't seem to work. What about moving the method to nikola.utils and calling it explicitly with a site instance?

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Jul 5, 2015

Member

Nah, let’s just keep it duplicated. Reverting and merging.

(Invariance failures are caused by this being branched out before rst.css and aria fixes.)

@Kwpolska Kwpolska self-assigned this Jul 5, 2015
@Kwpolska Kwpolska added the enhancement label Jul 5, 2015
This reverts commit ceda8e9.
@Kwpolska Kwpolska closed this Jul 5, 2015
@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

Thanks!

@Kwpolska Kwpolska deleted the unifying-compiler-extensions branch Jul 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.