Issue 6344080: plugin system experimental work
|
Created:
11 years, 10 months ago by Maiku Mori Modified:
11 years, 9 months ago Reviewers:
thomas.j.waldmann, Reimar Bauer Visibility:
Public. |
Descriptionplugin system experimental work
Patch Set 1 #
Total comments: 21
Patch Set 2 : Changes according to suggestions and small bugfixes. #
Total comments: 11
Patch Set 3 : Slightly hacky theme implementation. #
Total comments: 31
Patch Set 4 : Now it should fully work and also contains most of the suggestions from the last review. #
Total comments: 37
Patch Set 5 : Minor changes. #
Total comments: 3
Patch Set 6 : Minor changes #
Total comments: 8
MessagesTotal messages: 18
some comments http://codereview.appspot.com/6344080/diff/1/MoinMoin/app.py File MoinMoin/app.py (right): http://codereview.appspot.com/6344080/diff/1/MoinMoin/app.py#newcode38 MoinMoin/app.py:38: from MoinMoin import plugin should be moved before from MoinMoin.util.clock import Clock http://codereview.appspot.com/6344080/diff/1/MoinMoin/config/default.py File MoinMoin/config/default.py (right): http://codereview.appspot.com/6344080/diff/1/MoinMoin/config/default.py#newco... MoinMoin/config/default.py:502: ('enabled_plugins', None, 'List of plugin modules which are enabled.'), please use a prefix on one side and the same http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py#newcode11 MoinMoin/plugin.py:11: no extra empty lines http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py#newcode14 MoinMoin/plugin.py:14: from MoinMoin import log the logging module should be quite early used not after so much other imports http://codereview.appspot.com/6344080/diff/1/contrib/ExamplePlugin/setup.py File contrib/ExamplePlugin/setup.py (right): http://codereview.appspot.com/6344080/diff/1/contrib/ExamplePlugin/setup.py#n... contrib/ExamplePlugin/setup.py:38: 'moin>=2.0.0a0', why is that changed ?
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py#newcode14 MoinMoin/plugin.py:14: from MoinMoin import log On 2012/07/04 14:30:25, Reimar Bauer wrote: > the logging module should be quite early used not after so much other imports It's the first local package import, just like in app.py http://codereview.appspot.com/6344080/diff/1/contrib/ExamplePlugin/setup.py File contrib/ExamplePlugin/setup.py (right): http://codereview.appspot.com/6344080/diff/1/contrib/ExamplePlugin/setup.py#n... contrib/ExamplePlugin/setup.py:38: 'moin>=2.0.0a0', On 2012/07/04 14:30:25, Reimar Bauer wrote: > why is that changed ? It's explained in the commit, but seems like upload.py ignores commit messages. Basically setup tools think that "2.0.0a0" < "2.0" so it couldn't find my development moin2 install. This fixes it.
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/1/MoinMoin/_tests/test_plugin.py File MoinMoin/_tests/test_plugin.py (right): http://codereview.appspot.com/6344080/diff/1/MoinMoin/_tests/test_plugin.py#n... MoinMoin/_tests/test_plugin.py:1: # Copyright: 2012 MoinMoin:Miks Kalniņš <mikskalnins@maikumori.com> if possible, keep files ASCII http://codereview.appspot.com/6344080/diff/1/MoinMoin/_tests/test_plugin.py#n... MoinMoin/_tests/test_plugin.py:22: return "Test Plugin." properties maybe? http://codereview.appspot.com/6344080/diff/1/MoinMoin/app.py File MoinMoin/app.py (right): http://codereview.appspot.com/6344080/diff/1/MoinMoin/app.py#newcode104 MoinMoin/app.py:104: if app.cfg.enabled_plugins is not None: move this check to the loading function http://codereview.appspot.com/6344080/diff/1/MoinMoin/config/default.py File MoinMoin/config/default.py (right): http://codereview.appspot.com/6344080/diff/1/MoinMoin/config/default.py#newco... MoinMoin/config/default.py:502: ('enabled_plugins', None, 'List of plugin modules which are enabled.'), On 2012/07/04 14:30:25, Reimar Bauer wrote: > please use a prefix on one side and the same like plugins_enabled, plugins_foo http://codereview.appspot.com/6344080/diff/1/MoinMoin/config/default.py#newco... MoinMoin/config/default.py:503: ('plugin_settings', {}, 'Dictionary with plugin settings where the key is the plugin package name.'), as this is prone to grow to monstrous sizes, I'ld prefer handling it somehow differently, like giving params to plugin class constructors directly or so. needs more thinking... http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py#newcode36 MoinMoin/plugin.py:36: pass here you have properties, in the tests you call functions. properties preferred. http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py#newcode39 MoinMoin/plugin.py:39: def load_from_list(plugins, ignore_exceptions=True): do you intend to do other load_from_* ? otherwise maybe just use "load"? http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py#newcode53 MoinMoin/plugin.py:53: loaded_modules[plugin] = __import__(plugin, globals()) what from globals() do you need there? http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py#newcode70 MoinMoin/plugin.py:70: app = current_app() do you have this at the time you call this? http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py#newcode74 MoinMoin/plugin.py:74: cfg_key = plugin.__module__.split(".", 1) result of this looks like...? http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py#newcode76 MoinMoin/plugin.py:76: cfg = app.cfg.plugin_settings[cfg_key] antipattern alert! cfg = app.cfg.plugin_settings.get(cfg_key, {}) but see my other more general comment about this potentially huge dict. http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py#newcode87 MoinMoin/plugin.py:87: return initialized_list calling a dict a list is ... and please don't call it initialized_dict either, rather initialized(_plugins). http://codereview.appspot.com/6344080/diff/1/MoinMoin/plugin.py#newcode96 MoinMoin/plugin.py:96: run make test more often, esp. before submitting to codereview. http://codereview.appspot.com/6344080/diff/1/contrib/ExamplePlugin/ExampleMoi... File contrib/ExamplePlugin/ExampleMoinPlugin/__init__.py (right): http://codereview.appspot.com/6344080/diff/1/contrib/ExamplePlugin/ExampleMoi... contrib/ExamplePlugin/ExampleMoinPlugin/__init__.py:19: return "An example plugin for MoinMoin wiki engine to demonstrate some of the plugin systems features." properties?
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/1007/MoinMoin/_tests/test_plugin.py File MoinMoin/_tests/test_plugin.py (left): http://codereview.appspot.com/6344080/diff/1007/MoinMoin/_tests/test_plugin.p... MoinMoin/_tests/test_plugin.py:26: pytest.importorskip("ExampleMoinPlugin") Once we have some inbuilt plugins, this won't be needed. http://codereview.appspot.com/6344080/diff/1007/MoinMoin/_tests/test_plugin.py File MoinMoin/_tests/test_plugin.py (right): http://codereview.appspot.com/6344080/diff/1007/MoinMoin/_tests/test_plugin.p... MoinMoin/_tests/test_plugin.py:31: pytest.importorskip("ExampleMoinPlugin") Not really needed here, was deleted.
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/1007/contrib/ExamplePlugin/Example... File contrib/ExamplePlugin/ExampleMoinPlugin/__init__.py (left): http://codereview.appspot.com/6344080/diff/1007/contrib/ExamplePlugin/Example... contrib/ExamplePlugin/ExampleMoinPlugin/__init__.py:15: def author(self): forgot to change these, they're already changed in the last version
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/1007/MoinMoin/_tests/test_plugin.py File MoinMoin/_tests/test_plugin.py (right): http://codereview.appspot.com/6344080/diff/1007/MoinMoin/_tests/test_plugin.p... MoinMoin/_tests/test_plugin.py:17: description = "Test Plugin." unicode? http://codereview.appspot.com/6344080/diff/1007/MoinMoin/config/default.py File MoinMoin/config/default.py (right): http://codereview.appspot.com/6344080/diff/1007/MoinMoin/config/default.py#ne... MoinMoin/config/default.py:502: ('plugin_enabled_list', None, 'List of plugin modules which are enabled.'), don't put data types into names. if it is a list, just use plural. http://codereview.appspot.com/6344080/diff/1007/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6344080/diff/1007/MoinMoin/plugin.py#newcode51 MoinMoin/plugin.py:51: if plugins == None: "is" not "==" http://codereview.appspot.com/6344080/diff/1007/MoinMoin/plugin.py#newcode80 MoinMoin/plugin.py:80: cfg = plugin_config[package_name] still not fixed, see antipattern comment. http://codereview.appspot.com/6344080/diff/1007/contrib/ExamplePlugin/Example... File contrib/ExamplePlugin/ExampleMoinPlugin/__init__.py (right): http://codereview.appspot.com/6344080/diff/1007/contrib/ExamplePlugin/Example... contrib/ExamplePlugin/ExampleMoinPlugin/__init__.py:12: #def name(self): ? http://codereview.appspot.com/6344080/diff/1007/contrib/ExamplePlugin/Example... contrib/ExamplePlugin/ExampleMoinPlugin/__init__.py:13: name = "Example Plugin" u"..."? http://codereview.appspot.com/6344080/diff/1007/contrib/ExamplePlugin/Example... contrib/ExamplePlugin/ExampleMoinPlugin/__init__.py:16: return "Miks Kalnins" u"..."? http://codereview.appspot.com/6344080/diff/1007/contrib/ExamplePlugin/Example... contrib/ExamplePlugin/ExampleMoinPlugin/__init__.py:19: return "An example plugin for MoinMoin wiki engine to demonstrate some of the plugin systems features." u"..."?
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/11001/MoinMoin/app.py File MoinMoin/app.py (right): http://codereview.appspot.com/6344080/diff/11001/MoinMoin/app.py#newcode151 MoinMoin/app.py:151: loaders.append(app.jinja_env.loader) Reading this again and something is wrong here, it basically does nothing here, but still works. I'll look into that now.
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/11001/MoinMoin/app.py File MoinMoin/app.py (right): http://codereview.appspot.com/6344080/diff/11001/MoinMoin/app.py#newcode105 MoinMoin/app.py:105: logging.info("Plugins initialized: {0}".format(", ".join(app.initialized_plugins.keys()))) is .keys() needed here? http://codereview.appspot.com/6344080/diff/11001/MoinMoin/app.py#newcode151 MoinMoin/app.py:151: loaders.append(app.jinja_env.loader) where do you use loaders? you remove some code, but I don't see the replacement for it. http://codereview.appspot.com/6344080/diff/11001/MoinMoin/config/default.py File MoinMoin/config/default.py (right): http://codereview.appspot.com/6344080/diff/11001/MoinMoin/config/default.py#n... MoinMoin/config/default.py:341: ('theme_default', 'MoinMoin.themes.modernized.Modernized', "Default theme."), hmm, feels somehow like this should be rather a short name, not a fully qualified name and maybe not even bound to the name the code is using but just rather something declared in the extension. maybe also avoid uppercase. http://codereview.appspot.com/6344080/diff/11001/MoinMoin/config/default.py#n... MoinMoin/config/default.py:502: ('plugins_enabled', ["MoinMoin.themes.modernized"], 'List of plugin modules which are enabled.'), here the fully qualified thing is OK of course. http://codereview.appspot.com/6344080/diff/11001/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6344080/diff/11001/MoinMoin/plugin.py#newcode16 MoinMoin/plugin.py:16: from jinja2.loaders import FileSystemLoader our usual sorted import order: stdlib 3rd party moin stuff please keep it that way with one empty line between those groups. http://codereview.appspot.com/6344080/diff/11001/MoinMoin/plugin.py#newcode44 MoinMoin/plugin.py:44: # We'll use class name as unique identifier for now. if it is temporary, add TODO. http://codereview.appspot.com/6344080/diff/11001/MoinMoin/plugin.py#newcode54 MoinMoin/plugin.py:54: Influenced by Flask Themes (Matthew "LeafStorm" Frazier) (by ...) http://codereview.appspot.com/6344080/diff/11001/MoinMoin/plugin.py#newcode79 MoinMoin/plugin.py:79: self.path = os.path.dirname(os.path.abspath(sys.modules[self.__class__.__module__].__file__)) i could imagine this being a base class feature even. likely very many plugins will want to know their own path for some reason. it could be in form of a make_path(*more) method that does join(base_path, *more). http://codereview.appspot.com/6344080/diff/11001/MoinMoin/plugin.py#newcode139 MoinMoin/plugin.py:139: classes += subcls maybe instead of just taking the leaves, you could also take the classes that have some special attribute only present in the stuff we intend to use. so you could use both the base class and some child. e.g. if you slightly want to modify a plugin, but have both the original and the modification. http://codereview.appspot.com/6344080/diff/11001/MoinMoin/plugin.py#newcode145 MoinMoin/plugin.py:145: # ExampleMoinPlugin.SomeModule.SomePlugin -> ExampleMoinPlugin i don't think we need those comments, your names are good enough. almost. maybe use toplevel_package_name http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py File MoinMoin/themes/__init__.py (right): http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:20: from flask import _request_ctx_stack not used? http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:44: template = template[8:] what exactly are the 2 use cases that the above 2 lines manage? why are there 2? http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:49: raise TemplateNotFound(template) well, it is rather theme not found http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:57: fmt = '%s/%s' must be u'...' http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:59: res.extend(fmt.format(theme.identifier, t).encode("utf8") what's this encoding good for exactly? http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:65: #For faster lookup #<blank>comment http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:67: # Make all enabled theme static files servable. theme's http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:69: if isinstance(plugin, ThemeBase): maybe the plugin system could give some function that does that, like iter_plugins(ThemeBase) that only yields theme plugins. http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:73: #Lookup helpers. # ... http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:79: containable = lambda i: i if hasattr(i, '__contains__') else tuple(i) why is this needed? isn't it always a list what you get? or, if you think about it being a generator, couldn't you just wrap a list() around it? http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:83: def global_theme_template(ctx, templatename, fallback=True): isn't "global" (== generic) somehow contradicting "theme" (== specific) ? add a docstring explaining precisely what this does. http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:86: if (not fallback) or template_exists(templatepath): no parens http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:94: return url_for("serve.files", name=theme, filename=filename, _external=external) see above http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:106: print(app.initialized_plugins, theme_name) print ... (remove it before commit) http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/modernized/_... File MoinMoin/themes/modernized/__init__.py (right): http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/modernized/_... MoinMoin/themes/modernized/__init__.py:9: from __future__ import absolute_import, division if it is usually just about this little metadata, i don't think we'll need that import.
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/11001/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6344080/diff/11001/MoinMoin/plugin.py#newcode145 MoinMoin/plugin.py:145: # ExampleMoinPlugin.SomeModule.SomePlugin -> ExampleMoinPlugin On 2012/07/06 10:58:47, ThomasJWaldmann wrote: > i don't think we need those comments, your names are good enough. almost. maybe > use toplevel_package_name Isn't there only 1 package and all subfolders are modules? :P
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/2004/MoinMoin/auth/__init__.py File MoinMoin/auth/__init__.py (right): http://codereview.appspot.com/6344080/diff/2004/MoinMoin/auth/__init__.py#new... MoinMoin/auth/__init__.py:448: # logging.debug("session started for user {0!r}".format(userobj)) This is not part of the plugin system change and I'll cherry pick this out and commit it as bugfix. Basically sometimes userobj doesn't contain attributes needed for userobj.__repr__() (such as itemid and name) when that logging statement is called and that makes the whole system crash. Also this somehow didn't let me log in at all without crashing or raising exception, no idea why that happened, but this comment fixed it. Anyhow it's a separate issue.
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/2004/MoinMoin/auth/__init__.py File MoinMoin/auth/__init__.py (right): http://codereview.appspot.com/6344080/diff/2004/MoinMoin/auth/__init__.py#new... MoinMoin/auth/__init__.py:448: # logging.debug("session started for user {0!r}".format(userobj)) ok, just don't forget to remove this change before committing. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6344080/diff/2004/MoinMoin/plugin.py#newcode48 MoinMoin/plugin.py:48: path = os.path.dirname(os.path.abspath(sys.modules[self.__class__.__module__].__file__)) hmm, this is computed every time make_path is called. but the path is always the same. should be maybe done only once, in __init__ and put into self.plugin_dir or so. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/plugin.py#newcode131 MoinMoin/plugin.py:131: # Before we only took leaves which means there can't be Plugin class and another class which only leave classes http://codereview.appspot.com/6344080/diff/2004/MoinMoin/plugin.py#newcode133 MoinMoin/plugin.py:133: # else but this file because those ABCs will be interpreted as plugins and will raise exceptions. not sure i fully understood the problem... http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py File MoinMoin/themes/__init__.py (right): http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:43: template = template[8:] see last review. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:48: raise TemplateNotFound(template) see last review. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:56: fmt = "_themes/{0}/{1}" see last review. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:58: res.extend(fmt.format(theme.identifier, t).encode("utf8") see last review. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:65: #For faster lookup see last review. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:67: # Make all enabled theme static files servable. see last review. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:69: if isinstance(plugin, ThemeBase): see last review. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:73: #Lookup helpers. see last review. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:79: containable = lambda i: i if hasattr(i, '__contains__') else tuple(i) see last review. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:83: def global_theme_template(ctx, templatename, fallback=True): see last review. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:86: if (not fallback) or template_exists(templatepath): see last review. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:94: return url_for("serve.files", name=theme, filename=filename, _external=external) see last review. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:117: #Try using current themes template. # ...
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py File MoinMoin/themes/__init__.py (right): http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:83: def global_theme_template(ctx, templatename, fallback=True): On 2012/07/06 10:58:47, ThomasJWaldmann wrote: > isn't "global" (== generic) somehow contradicting "theme" (== specific) ? > > add a docstring explaining precisely what this does. These are the two functions which are added to jinja2 template globals so they can be accessed from templates using "theme" and "theme_static" functions. They're global because they're accessible in all templates. Any suggestions for better name? http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:86: if (not fallback) or template_exists(templatepath): On 2012/07/06 10:58:47, ThomasJWaldmann wrote: > no parens Done. http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:94: return url_for("serve.files", name=theme, filename=filename, _external=external) On 2012/07/06 10:58:47, ThomasJWaldmann wrote: > see above Done. http://codereview.appspot.com/6344080/diff/11001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:106: print(app.initialized_plugins, theme_name) On 2012/07/06 10:58:47, ThomasJWaldmann wrote: > print ... (remove it before commit) Done. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py File MoinMoin/themes/__init__.py (right): http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:43: template = template[8:] On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > see last review. When templates are looked up, it first tries to look up theme template which is named according to this format "_themes/{theme}/{template}" so we're removing "_themes" part and we try to extract theme and template from the rest of the string. Other case is when the string doesn't start with "_themes", one might be referring to specific theme directly. If all fails and it can't look up the theme, the app will use next Loader in chain which typically is the default one which loads from default templates. I could change this loader to automatically fail to default and that way eliminate the need to chain loaders in most cases. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:48: raise TemplateNotFound(template) On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > see last review. In most cases it indeed is ThemeNotFound, but not all. This also is the exception which Loaders usually raise so to maintain compatibility with other code which might interact with loaders I use TemplateNotFound exception which at the end is also correct. Loaders are not aware of themes, all they care about is mapping template names to template paths. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:56: fmt = "_themes/{0}/{1}" On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > see last review. Done. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:58: res.extend(fmt.format(theme.identifier, t).encode("utf8") On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > see last review. Done. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:65: #For faster lookup On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > see last review. Done. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:67: # Make all enabled theme static files servable. On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > see last review. Done. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:69: if isinstance(plugin, ThemeBase): On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > see last review. From chat: <MaikuMori> it has most of the suggestions, I still need to research that jinja method you mentioned and I'll work on the plugin lookup by type and (maybe) short idetificator a bit later when I have more plugins. (I'll try to reply here from now on to make it easier and in one place) http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:73: #Lookup helpers. On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > see last review. Done. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:79: containable = lambda i: i if hasattr(i, '__contains__') else tuple(i) On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > see last review. Done.
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py File MoinMoin/themes/__init__.py (right): http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:43: template = template[8:] I still don't understand the difference between the cases with and without "_themes" prefix or why we need both cases. can you give an example? in the end, this should be documented in the code, so it is maintainable. such stuff as this was the main reason (at least for my part), why flask-themes could not get fixed - just too many strange unclarities. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:48: raise TemplateNotFound(template) ok, so please add a comment to the code explaining this. http://codereview.appspot.com/6344080/diff/14002/MoinMoin/themes/__init__.py File MoinMoin/themes/__init__.py (right): http://codereview.appspot.com/6344080/diff/14002/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:116: #Try using current themes template. # ...
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py File MoinMoin/themes/__init__.py (right): http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:43: template = template[8:] On 2012/07/11 13:31:04, ThomasJWaldmann wrote: > I still don't understand the difference between the cases with and without > "_themes" prefix or why we need both cases. > > can you give an example? > > in the end, this should be documented in the code, so it is maintainable. such > stuff as this was the main reason (at least for my part), why flask-themes could > not get fixed - just too many strange unclarities. I agree, I would probably do this a bit differently, but I didn't want to break anything already in templates. Not quite sure if we need the two cases here but: 1st use case - You can just use theme("sometemplate.html"), which will try to look up theme template (using _theme) and then fall back to inbuilt templates 2nd use case - You can link directly to the theme template using string such as "themename/template.html". This basically allows you to use other template files even if user has different template selected.
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/2004/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6344080/diff/2004/MoinMoin/plugin.py#newcode131 MoinMoin/plugin.py:131: # Before we only took leaves which means there can't be Plugin class and another class which only On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > leave classes leaf classes? http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py File MoinMoin/themes/__init__.py (right): http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:43: template = template[8:] add such stuff to the docstrings. http://codereview.appspot.com/6344080/diff/14002/MoinMoin/themes/__init__.py File MoinMoin/themes/__init__.py (right): http://codereview.appspot.com/6344080/diff/14002/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:116: #Try using current themes template. On 2012/07/11 13:31:05, ThomasJWaldmann wrote: > # ... it meant "see last review" if that was unclear. always add a blank after #.
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/2004/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6344080/diff/2004/MoinMoin/plugin.py#newcode48 MoinMoin/plugin.py:48: path = os.path.dirname(os.path.abspath(sys.modules[self.__class__.__module__].__file__)) On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > hmm, this is computed every time make_path is called. but the path is always the > same. should be maybe done only once, in __init__ and put into self.plugin_dir > or so. If I do it in init and plugin overwrites init it won't work unless they use super(). Is the minor optimization worth having to remember to call super()? http://codereview.appspot.com/6344080/diff/2004/MoinMoin/plugin.py#newcode131 MoinMoin/plugin.py:131: # Before we only took leaves which means there can't be Plugin class and another class which only On 2012/07/31 19:38:39, ThomasJWaldmann wrote: > On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > > leave classes > > leaf classes? Done. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/plugin.py#newcode133 MoinMoin/plugin.py:133: # else but this file because those ABCs will be interpreted as plugins and will raise exceptions. On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > not sure i fully understood the problem... I need to somehow filter out all the ABCs, right now I just compare all the classes to classes declared in this file (All of them are ABCs). If ABC is declared somewhere else it won't filter it out and threat as plugin which will cause exception because the parameter/methods aren't implemented in it. Is there a better way to filter them out? http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py File MoinMoin/themes/__init__.py (right): http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:43: template = template[8:] On 2012/07/31 19:38:39, ThomasJWaldmann wrote: > add such stuff to the docstrings. Done. http://codereview.appspot.com/6344080/diff/2004/MoinMoin/themes/__init__.py#n... MoinMoin/themes/__init__.py:117: #Try using current themes template. On 2012/07/11 08:42:23, ThomasJWaldmann wrote: > # ... Done. http://codereview.appspot.com/6344080/diff/14002/MoinMoin/themes/__init__.py File MoinMoin/themes/__init__.py (right): http://codereview.appspot.com/6344080/diff/14002/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:116: #Try using current themes template. On 2012/07/31 19:38:39, ThomasJWaldmann wrote: > On 2012/07/11 13:31:05, ThomasJWaldmann wrote: > > # ... > > it meant "see last review" if that was unclear. always add a blank after #. Done.
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/19001/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6344080/diff/19001/MoinMoin/plugin.py#newcode130 MoinMoin/plugin.py:130: # XXX: This potentialy has problems. "potentially" what are the problems? the stuff described below or anything else? http://codereview.appspot.com/6344080/diff/19001/MoinMoin/themes/__init__.py File MoinMoin/themes/__init__.py (right): http://codereview.appspot.com/6344080/diff/19001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:41: Supports two ways of refering to templates: "referring" (this is not the W3C :D ). http://codereview.appspot.com/6344080/diff/19001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:44: - You can link directly to the theme template using string such as using a string http://codereview.appspot.com/6344080/diff/19001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:46: even if user has different template selected. if the user has a ...
Sign in to reply to this message.
http://codereview.appspot.com/6344080/diff/19001/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6344080/diff/19001/MoinMoin/plugin.py#newcode130 MoinMoin/plugin.py:130: # XXX: This potentialy has problems. On 2012/08/01 13:59:21, ThomasJWaldmann wrote: > "potentially" > what are the problems? the stuff described below or anything else? Yes, those described below. http://codereview.appspot.com/6344080/diff/19001/MoinMoin/themes/__init__.py File MoinMoin/themes/__init__.py (right): http://codereview.appspot.com/6344080/diff/19001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:41: Supports two ways of refering to templates: On 2012/08/01 13:59:21, ThomasJWaldmann wrote: > "referring" (this is not the W3C :D ). Done. http://codereview.appspot.com/6344080/diff/19001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:44: - You can link directly to the theme template using string such as On 2012/08/01 13:59:21, ThomasJWaldmann wrote: > using a string Done. http://codereview.appspot.com/6344080/diff/19001/MoinMoin/themes/__init__.py#... MoinMoin/themes/__init__.py:46: even if user has different template selected. On 2012/08/01 13:59:21, ThomasJWaldmann wrote: > if the user has a ... Done.
Sign in to reply to this message.
|
