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

WordPress compiler plugin. #91

Merged
merged 20 commits into from Jul 5, 2015
Merged

WordPress compiler plugin. #91

merged 20 commits into from Jul 5, 2015

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Jun 27, 2015

This plugin allows to compile WordPress posts (i.e. posts directly copied out of a WordPress blog, without any modifications).

It has support for shortcodes. By default, it only supports the [code] shortcode, but plugins for other shortcodes can be added via a plugin interface. See wordpress/plugins/code.py for an example.

Review on Reviewable

__all__ = ['CompileWordpress', 'WordPressPlugin']


class WordPressPlugin(IPlugin):
Copy link
Member

@ralsina ralsina Jul 2, 2015

Choose a reason for hiding this comment

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

Why not make this a PageCompiler? Look at https://github.com/getnikola/nikola/blob/master/nikola/plugins/compile/pandoc.py#L43 for an example.

Copy link
Contributor Author

@felixfontein felixfontein Jul 2, 2015

Choose a reason for hiding this comment

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

This is a class which shortcode plugins for the WordPress compiler should derive from. This is not the class for the WordPress page compiler itself. The WordPress page compiler is CompileWordpress and defined in wordpress.py.

Copy link
Member

@ralsina ralsina Jul 2, 2015

Choose a reason for hiding this comment

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

In any case, inherit at least nikola.plugin_categories.BasePlugin or you will be missing stuff.

Copy link
Contributor Author

@felixfontein felixfontein Jul 2, 2015

Choose a reason for hiding this comment

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

Why? The plugin handling for .wpplugin plugins (which inherit from WordPressPlugin) is done completely by the WordPress page compiler plugin. Nikola won't see these plugins.

Copy link
Member

@ralsina ralsina Jul 2, 2015

Choose a reason for hiding this comment

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

Oh, it will. Nikola's plugin loader will find them, unless you put them in a different place? Then they will be loaded twice.
Also, there's no reason for the compiler to load plugins, just let Nikola load them and get them from self.site.plugin_manager. Saves you a chunk of code, too.

Copy link
Contributor Author

@felixfontein felixfontein Jul 2, 2015

Choose a reason for hiding this comment

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

How about a "plugin plugin" category? The base class for that category would have a field for the plugin name this plugin is for, and the Nikola main object would get a function which allows to retrieve all plugins for a given plugin.
I can add something like that to Nikola if you want. The problem is that this won't be part of Nikola anytime soon (7.6.0 was just released), and so the WordPress plugin won't be useable until a newer Nikola version is released -- and in particular, it won't be useable for almost all v7 versions.

Copy link
Contributor Author

@felixfontein felixfontein Jul 2, 2015

Choose a reason for hiding this comment

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

Re registering plugin categories from within plugins: that would be really cumbersome, since we have to specify the plugin categories before any plugin is loaded. Except of course if we load plugins twice, but I don't really like that idea.

Copy link
Member

@Kwpolska Kwpolska Jul 3, 2015

Choose a reason for hiding this comment

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

class CompilerExtension(BasePlugin):
    compiler_name = ''

class RestExtension(CompilerExtension):
    compiler_name = 'rest'

class MarkdownExtension(CompilerExtension)
    compiler_name = 'markdown'

Your plugins would be CompilerExtensions with compiler_name = 'wordpress', and would filter for that.

We could make your plugin v8-only (and we really need to get our act together and land that release) to make compatibility easier.

Copy link
Contributor Author

@felixfontein felixfontein Jul 3, 2015

Choose a reason for hiding this comment

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

Yes, that's what I was thinking. Also very good idea to include RestExtension and MarkdownExtension into this scheme!

Apropos v8: is there a timeline when v8 should be done? For the earlytask branch, the main problem is that it is waiting for some features to appear in official doit before it an be merged.

Copy link
Member

@Kwpolska Kwpolska Jul 4, 2015

Choose a reason for hiding this comment

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

Well, it will take a while for v8 to become a thing, because there isn’t that much to change yet. However, I just implemented CompilerExtensions in getnikola/nikola@c65be6e. You could make your compiler v7.6.1+ and use them.

@ralsina
Copy link
Member

ralsina commented Jul 2, 2015

Review status: 0 of 10 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.


v7/wordpress_compiler/README.md, line 12 [r1] (raw file):
If you put this in conf.py.sample it will be automatically shown here, and also shown as a suggestion to the user when he installs the plugin. For example here's what happens when you install markmin:

$ nikola plugin -i markmin
INFO:requests.packages.urllib3.connectionpool:Starting new HTTPS connection (1): plugins.getnikola.com
[2015-07-02T13:49:30Z] INFO: plugin: Downloading: https://plugins.getnikola.com/v7/markmin.zip
INFO:requests.packages.urllib3.connectionpool:Starting new HTTPS connection (1): plugins.getnikola.com
[2015-07-02T13:49:31Z] INFO: plugin: Extracting: markmin into plugins/
[2015-07-02T13:49:31Z] NOTICE: plugin: This plugin has a sample config file.  Integrate it with yours in order to make this plugin work!
Contents of the conf.py.sample file:

    # Add the markmin compiler to your COMPILERS dict.
    COMPILERS["markmin"] = ('.mm',)

    # Add markmin files to your POSTS, PAGES
    POSTS = POSTS + (("posts/*.mm", "posts", "post.tmpl"),)
    PAGES = PAGES + (("stories/*.mm", "posts", "post.tmpl"),)


v7/wordpress_compiler/wordpress/plugins/code.py, line 3 [r1] (raw file):
Since you wrote it, you shoud use your name in it :-)


v7/wordpress_compiler/wordpress/plugins/code.wpplugin, line 2 [r1] (raw file):
this name is waaaaay too generic and bound to conflict with something.


Comments from the review on Reviewable.io

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 2, 2015

Since you wrote it, you shoud use your name in it :-)

Well, I patched it together, but I took some parts from other people -- like you ;-)

this name is waaaaay too generic and bound to conflict with something.

Isn't this always local to the .wpplugin file?

@ralsina
Copy link
Member

ralsina commented Jul 2, 2015

I try to keep the names consistent, same name in the metadata plugin file, the plugin itself, task names, etc. You never know what someone is going to try in the future :-)

@ralsina
Copy link
Member

ralsina commented Jul 3, 2015

Ok, let's merge this whenever you want, and let's revisit it in a few months.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 3, 2015

I'll add an remark for potential shortcode plugin authors that the plugin interface will be changed for v8, so they should be aware that their plugins have to be changed a bit.
I'll probably manage that by tomorrow.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

The new version now uses the CompilerExtension interface of Nikola 7.6.1.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

I guess the tests will fail until Nikola 7.6.1 is released.

from .wordpress import CompileWordpress


__all__ = ['CompileWordpress', 'WordPressPlugin']
Copy link
Member

@Kwpolska Kwpolska Jul 5, 2015

Choose a reason for hiding this comment

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

  • should be a tuple, not a list
  • where is WordPressPlugin?

Copy link
Contributor Author

@felixfontein felixfontein Jul 5, 2015

Choose a reason for hiding this comment

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

Thanks! I moved WordPressPlugin to plugin_interface, otherwise yapsy would (at least for me) load that definition as the plugin and ignore the page compiler...

@Kwpolska
Copy link
Member

Kwpolska commented Jul 5, 2015

  • Replace by the contributors with , Felix Fontein.
  • Add a # -*- coding: utf-8 -*- declaration.
  • To fix the tests, you must duplicate even more code, the plugin test suite uses its own base.py file.
  • Get rid of pickles
  • Get rid of UnformattedLexer

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

Ok, there's a big problem with the new CompilerExtensions interface. How do I now import something from the original WordPress plugin? When it was using its own plugin system, I would just write import wordpress.xxx and everything would work fine. Now, I can't anymore.

import pygments.token


class UnformattedLexer(pygments.lexer.RegexLexer):
Copy link
Member

@Kwpolska Kwpolska Jul 5, 2015

Choose a reason for hiding this comment

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

Do you really need that? There is a text lexer available.

Copy link
Contributor Author

@felixfontein felixfontein Jul 5, 2015

Choose a reason for hiding this comment

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

Must have missed that one.

Copy link
Contributor Author

@felixfontein felixfontein Jul 5, 2015

Choose a reason for hiding this comment

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

Done.

@Kwpolska
Copy link
Member

Kwpolska commented Jul 5, 2015

You could cheat and use import plugins.wordpress.plugin_interface, though this will make user-wide plugin installs impossible.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

Doesn't work for me, either, since I'm including the wordpress plugin via EXTRA_PLUGINS_DIRS.
I'll Maybe I can expose the modules dynamically in the register method. Will be somewhat ugly, but should work. I'll play with that later...

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

Hmm, that won't work either, since the first thing I need to do is import wordpress.plugin_interface to be able to use wordpress.plugin_interface.WordPressPlugin. That sucks bigtime.

@Kwpolska
Copy link
Member

Kwpolska commented Jul 5, 2015

Hack idea 2: use the plain CompilerExtension class and set compiler_name = 'wordpress'.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

And define register() myself, and never be able to define any helper functionality in the plugin object itself. Very, very hacky. But probably the only thing that works... :(

@Kwpolska
Copy link
Member

Kwpolska commented Jul 5, 2015

You could write some functions (≠methods) and pass them to every extension in your compiler, when you register them.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

I'm now providing the modules. I'll do some more testing by porting my other plugins to this way...

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

Seems to work this way.

@Kwpolska
Copy link
Member

Kwpolska commented Jul 5, 2015

Please complete the checklist a few posts above.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

I now also adjusted the copyright lines (though I didn't do precisely the replacement you wanted to do). Is that ok?

@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

Yeah, just noticed that too :)

Kwpolska added a commit that referenced this pull request Jul 5, 2015
@Kwpolska Kwpolska merged commit ea80c63 into getnikola:master Jul 5, 2015
@felixfontein
Copy link
Contributor Author

felixfontein commented Jul 5, 2015

Thanks!

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.

None yet

3 participants