Issue 6449054: Stores plugins
|
Created:
12 years, 5 months ago by Maiku Mori Modified:
12 years, 5 months ago Reviewers:
thomas.j.waldmann Visibility:
Public. |
DescriptionStores plugins
Patch Set 1 #
Total comments: 10
Patch Set 2 : Different variation #
Total comments: 20
Patch Set 3 : Minor changes #Patch Set 4 : #
MessagesTotal messages: 9
well, i don't think it can be done this way, as it would ONLY work for simple mappings then. http://codereview.appspot.com/6449054/diff/1/MoinMoin/app.py File MoinMoin/app.py (right): http://codereview.appspot.com/6449054/diff/1/MoinMoin/app.py#newcode115 MoinMoin/app.py:115: ) no, that can't be done here. create_simple_mapping is only one way to setup the storage. obviously you could also create less simple mappings. http://codereview.appspot.com/6449054/diff/1/MoinMoin/config/default.py File MoinMoin/config/default.py (right): http://codereview.appspot.com/6449054/diff/1/MoinMoin/config/default.py#newco... MoinMoin/config/default.py:124: raise error.ConfigurationError("No acl configuration specified! You need to define a user_profile_acl. " this is also a bit less clean than the way it was before. you'ld have to introduce more and more config attributes with this x_y_z name pattern instead of having simpler function/constructor parameters. http://codereview.appspot.com/6449054/diff/1/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6449054/diff/1/MoinMoin/plugin.py#newcode82 MoinMoin/plugin.py:82: class StoreBase(PluginBase): somehow it feels a bit strange to have this here in "plugins" and not in storage package. http://codereview.appspot.com/6449054/diff/1/MoinMoin/plugin.py#newcode90 MoinMoin/plugin.py:90: pass i think i already mentioned it: use docstrings at such places and NOT "pass". http://codereview.appspot.com/6449054/diff/1/MoinMoin/plugin.py#newcode98 MoinMoin/plugin.py:98: pass you don't need "pass", the docstring is enough to make it non-empty. http://codereview.appspot.com/6449054/diff/1/MoinMoin/storage/__init__.py File MoinMoin/storage/__init__.py (right): http://codereview.appspot.com/6449054/diff/1/MoinMoin/storage/__init__.py#new... MoinMoin/storage/__init__.py:36: raise ValueError("malformed backend uri: {0}".format(backend_name_uri)) i don't see why you changed this http://codereview.appspot.com/6449054/diff/1/MoinMoin/storage/backends/stores.py File MoinMoin/storage/backends/stores.py (right): http://codereview.appspot.com/6449054/diff/1/MoinMoin/storage/backends/stores... MoinMoin/storage/backends/stores.py:52: found = store if you break, you have it in "store". for the case it does not find it, use the else branch of for...else. http://codereview.appspot.com/6449054/diff/1/MoinMoin/storage/stores/fs.py File MoinMoin/storage/stores/fs.py (right): http://codereview.appspot.com/6449054/diff/1/MoinMoin/storage/stores/fs.py#ne... MoinMoin/storage/stores/fs.py:92: description = u"moin2's builtin store" description should be descriptive :)
Sign in to reply to this message.
http://codereview.appspot.com/6449054/diff/1/MoinMoin/storage/__init__.py File MoinMoin/storage/__init__.py (right): http://codereview.appspot.com/6449054/diff/1/MoinMoin/storage/__init__.py#new... MoinMoin/storage/__init__.py:36: raise ValueError("malformed backend uri: {0}".format(backend_name_uri)) Fixed a bug here, backend_uri is undefined at that point, closest thing we have is backend_name_uri.
Sign in to reply to this message.
http://codereview.appspot.com/6449054/diff/1/MoinMoin/storage/__init__.py File MoinMoin/storage/__init__.py (right): http://codereview.appspot.com/6449054/diff/1/MoinMoin/storage/__init__.py#new... MoinMoin/storage/__init__.py:36: raise ValueError("malformed backend uri: {0}".format(backend_name_uri)) if it is saying "malformed" to the uri given by the user, it should give the uri as given by the user.
Sign in to reply to this message.
you should also show how it looks like without using create_simple_mapping. also, if you change this, you need to update the docs also. http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py File MoinMoin/app.py (right): http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py#newcode105 MoinMoin/app.py:105: app.initialized_plugins = plugin.init_all_plugins(app.cfg.plugins_config, False) what was the problem with making one call out of these 2 lines and doing it in the config? http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py#newcode108 MoinMoin/app.py:108: # mappings if a comment says less than the commented code says anyway, you can leave it away. http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py#newcode112 MoinMoin/app.py:112: app.cfg.mappings['acls'], **app.cfg.mappings (maybe needs some other changes also) http://codereview.appspot.com/6449054/diff/7001/MoinMoin/config/default.py File MoinMoin/config/default.py (right): http://codereview.appspot.com/6449054/diff/7001/MoinMoin/config/default.py#ne... MoinMoin/config/default.py:116: raise error.ConfigurationError("No mapping configuration specified! You need to define a mappings. " ... a mapping? http://codereview.appspot.com/6449054/diff/7001/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6449054/diff/7001/MoinMoin/plugin.py#newcode86 MoinMoin/plugin.py:86: """ if you do not want to add more to the docstring now, kill the empty line in it. http://codereview.appspot.com/6449054/diff/7001/MoinMoin/storage/stores/fs.py File MoinMoin/storage/stores/fs.py (right): http://codereview.appspot.com/6449054/diff/7001/MoinMoin/storage/stores/fs.py... MoinMoin/storage/stores/fs.py:92: description = u"MoinMoin's builtin filesystem store. Stores data in a file directory." "file directory" somehow sounds strange. somehow the second sentence is redundant with the first anyway. http://codereview.appspot.com/6449054/diff/7001/wikiconfig.py File wikiconfig.py (right): http://codereview.appspot.com/6449054/diff/7001/wikiconfig.py#newcode12 wikiconfig.py:12: from MoinMoin.storage import create_simple_mapping why did you change import order for the above 2 imports? is it needed?
Sign in to reply to this message.
As for example, I could add it to the docs. You can either set the values directly in config or create a helper function such as create_simple_mapping. http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py File MoinMoin/app.py (right): http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py#newcode105 MoinMoin/app.py:105: app.initialized_plugins = plugin.init_all_plugins(app.cfg.plugins_config, False) On 2012/07/31 19:34:00, ThomasJWaldmann wrote: > what was the problem with making one call out of these 2 lines and doing it in > the config? Then config has to be very structured (in specific order), plugins can't access every single config value. http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py#newcode108 MoinMoin/app.py:108: # mappings On 2012/07/31 19:34:00, ThomasJWaldmann wrote: > if a comment says less than the commented code says anyway, you can leave it > away. Done. http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py#newcode112 MoinMoin/app.py:112: app.cfg.mappings['acls'], On 2012/07/31 19:34:00, ThomasJWaldmann wrote: > **app.cfg.mappings (maybe needs some other changes also) Can't do that if it's fallowed by another variable, should I reorder parameters? I quite like that it says which values are passed. http://codereview.appspot.com/6449054/diff/7001/MoinMoin/plugin.py File MoinMoin/plugin.py (right): http://codereview.appspot.com/6449054/diff/7001/MoinMoin/plugin.py#newcode86 MoinMoin/plugin.py:86: """ On 2012/07/31 19:34:00, ThomasJWaldmann wrote: > if you do not want to add more to the docstring now, kill the empty line in it. Done. http://codereview.appspot.com/6449054/diff/7001/MoinMoin/storage/stores/fs.py File MoinMoin/storage/stores/fs.py (right): http://codereview.appspot.com/6449054/diff/7001/MoinMoin/storage/stores/fs.py... MoinMoin/storage/stores/fs.py:92: description = u"MoinMoin's builtin filesystem store. Stores data in a file directory." On 2012/07/31 19:34:00, ThomasJWaldmann wrote: > "file directory" somehow sounds strange. somehow the second sentence is > redundant with the first anyway. Done. http://codereview.appspot.com/6449054/diff/7001/wikiconfig.py File wikiconfig.py (right): http://codereview.appspot.com/6449054/diff/7001/wikiconfig.py#newcode12 wikiconfig.py:12: from MoinMoin.storage import create_simple_mapping On 2012/07/31 19:34:00, ThomasJWaldmann wrote: > why did you change import order for the above 2 imports? > > is it needed? Done.
Sign in to reply to this message.
http://codereview.appspot.com/6449054/diff/7001/MoinMoin/config/default.py File MoinMoin/config/default.py (right): http://codereview.appspot.com/6449054/diff/7001/MoinMoin/config/default.py#ne... MoinMoin/config/default.py:116: raise error.ConfigurationError("No mapping configuration specified! You need to define a mappings. " On 2012/07/31 19:34:00, ThomasJWaldmann wrote: > ... a mapping? Done.
Sign in to reply to this message.
http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py File MoinMoin/app.py (right): http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py#newcode105 MoinMoin/app.py:105: app.initialized_plugins = plugin.init_all_plugins(app.cfg.plugins_config, False) having some specific order is no problem, that would just need to put that way into a sample config (and into the docs). not sure what exactly the problem is with accessing config values. http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py#newcode112 MoinMoin/app.py:112: app.cfg.mappings['acls'], well, to the reader of the code, it says what it is accessing. to the called function, only order matters (as it is now). with **kw method, it would give kwargs to that function, which would be easier to extend. of course for this way, best is if everything is given as a named argument. http://codereview.appspot.com/6449054/diff/7001/MoinMoin/config/default.py File MoinMoin/config/default.py (right): http://codereview.appspot.com/6449054/diff/7001/MoinMoin/config/default.py#ne... MoinMoin/config/default.py:116: raise error.ConfigurationError("No mapping configuration specified! You need to define a mappings. " Grammar is still strange. "You need to define a mapping configuration." (plus "a") is the blank at the end needed?
Sign in to reply to this message.
http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py File MoinMoin/app.py (right): http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py#newcode105 MoinMoin/app.py:105: app.initialized_plugins = plugin.init_all_plugins(app.cfg.plugins_config, False) On 2012/08/01 13:54:19, ThomasJWaldmann wrote: > having some specific order is no problem, that would just need to put that way > into a sample config (and into the docs). > > not sure what exactly the problem is with accessing config values. So far everything works like this, is there a reason to move it? If it's needed, I think it would be best to refactor it once I have all the branches merged as they all depend on the core plugin code. http://codereview.appspot.com/6449054/diff/7001/MoinMoin/app.py#newcode112 MoinMoin/app.py:112: app.cfg.mappings['acls'], On 2012/08/01 13:54:19, ThomasJWaldmann wrote: > well, to the reader of the code, it says what it is accessing. to the called > function, only order matters (as it is now). with **kw method, it would give > kwargs to that function, which would be easier to extend. of course for this > way, best is if everything is given as a named argument. Done. http://codereview.appspot.com/6449054/diff/7001/MoinMoin/config/default.py File MoinMoin/config/default.py (right): http://codereview.appspot.com/6449054/diff/7001/MoinMoin/config/default.py#ne... MoinMoin/config/default.py:116: raise error.ConfigurationError("No mapping configuration specified! You need to define a mappings. " On 2012/08/01 13:54:19, ThomasJWaldmann wrote: > Grammar is still strange. > > "You need to define a mapping configuration." (plus "a") > is the blank at the end needed? Done and, yes, since it's a split string and you need a space between sentences.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
