namespace - item_root mapping

namespace - item_root mapping

[Thomas.J.Waldmann, 2013-07-25 11:16:16]

https://codereview.appspot.com/11584044/diff/1/MoinMoin/config/default.py
File MoinMoin/config/default.py (right):

https://codereview.appspot.com/11584044/diff/1/MoinMoin/config/default.py#new...
MoinMoin/config/default.py:462: ('item_root', None, "Name of the root item (aka
'front page'). [Unicode]"),
why do you keep this setting?

https://codereview.appspot.com/11584044/diff/1/MoinMoin/config/default.py#new...
MoinMoin/config/default.py:463: ('root_mapping', None, "mapping of namespaces to
item_roots." ),
why do you have a default here that does not work?

https://codereview.appspot.com/11584044/diff/1/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

https://codereview.appspot.com/11584044/diff/1/MoinMoin/items/__init__.py#new...
MoinMoin/items/__init__.py:298: fqname = CompositeName(fqname.namespace,
NAME_EXACT, app.cfg.root_mapping[fqname.namespace])
as the default root_mapping is None, it will raise an exception here.

https://codereview.appspot.com/11584044/diff/1/wikiconfig.py
File wikiconfig.py (right):

https://codereview.appspot.com/11584044/diff/1/wikiconfig.py#newcode49
wikiconfig.py:49: item_root = root_mapping[NAMESPACE_DEFAULT] # Front Page
did you search the code for places using item_root?

check if they still make sense or whether they should also rather use
root_mapping.

[ashu1461, 2013-07-25 16:31:33]

https://codereview.appspot.com/11584044/diff/1/MoinMoin/config/default.py
File MoinMoin/config/default.py (right):

https://codereview.appspot.com/11584044/diff/1/MoinMoin/config/default.py#new...
MoinMoin/config/default.py:462: ('item_root', None, "Name of the root item (aka
'front page'). [Unicode]"),
On 2013/07/25 11:16:17, Thomas.J.Waldmann wrote:
> why do you keep this setting?

The code uses item_root at lots of places, so I kept it this way.
Plus it would be convenient to have a something like default_root (that can be
used directly.)

https://codereview.appspot.com/11584044/diff/1/MoinMoin/config/default.py#new...
MoinMoin/config/default.py:463: ('root_mapping', None, "mapping of namespaces to
item_roots." ),
On 2013/07/25 11:16:17, Thomas.J.Waldmann wrote:
> why do you have a default here that does not work?

We later fill this up in wikiconfig.py .

https://codereview.appspot.com/11584044/diff/1/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

https://codereview.appspot.com/11584044/diff/1/MoinMoin/items/__init__.py#new...
MoinMoin/items/__init__.py:298: fqname = CompositeName(fqname.namespace,
NAME_EXACT, app.cfg.root_mapping[fqname.namespace])
On 2013/07/25 11:16:17, Thomas.J.Waldmann wrote:
> as the default root_mapping is None, it will raise an exception here.

ok. if that key exists in mapping use the value else use u''.

https://codereview.appspot.com/11584044/diff/1/wikiconfig.py
File wikiconfig.py (right):

https://codereview.appspot.com/11584044/diff/1/wikiconfig.py#newcode49
wikiconfig.py:49: item_root = root_mapping[NAMESPACE_DEFAULT] # Front Page
On 2013/07/25 11:16:17, Thomas.J.Waldmann wrote:
> did you search the code for places using item_root?
> 
> check if they still make sense or whether they should also rather use
> root_mapping.

Yes checked.

[Thomas.J.Waldmann, 2013-07-25 16:39:40]

https://codereview.appspot.com/11584044/diff/1/MoinMoin/config/default.py
File MoinMoin/config/default.py (right):

https://codereview.appspot.com/11584044/diff/1/MoinMoin/config/default.py#new...
MoinMoin/config/default.py:462: ('item_root', None, "Name of the root item (aka
'front page'). [Unicode]"),
> The code uses item_root at lots of places, so I kept it this way.

If the code is using that at a lot of places, likely they need to get changed so
they support not just the default namespace redirect, but redirects for all
namespaces as you configure it in that other setting.

> Plus it would be convenient to have a something 
> like default_root (that can be used directly.)

That would be just the application root.

Also: this None does not work either as a default.

https://codereview.appspot.com/11584044/diff/1/MoinMoin/config/default.py#new...
MoinMoin/config/default.py:463: ('root_mapping', None, "mapping of namespaces to
item_roots." ),
wikiconfig.py should not require configuring something if there can be a good
default.

https://codereview.appspot.com/11584044/diff/1/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

https://codereview.appspot.com/11584044/diff/1/MoinMoin/items/__init__.py#new...
MoinMoin/items/__init__.py:298: fqname = CompositeName(fqname.namespace,
NAME_EXACT, app.cfg.root_mapping[fqname.namespace])
> ok. if that key exists in mapping use the value else use u''.

How is u'' helpful?

https://codereview.appspot.com/11584044/diff/1/wikiconfig.py
File wikiconfig.py (right):

https://codereview.appspot.com/11584044/diff/1/wikiconfig.py#newcode49
wikiconfig.py:49: item_root = root_mapping[NAMESPACE_DEFAULT] # Front Page

> Yes checked.

And the outcome was that they don't need change?

That sounds not very logical / not very consistent.

[Thomas.J.Waldmann, 2013-07-30 21:57:52]

https://codereview.appspot.com/11584044/diff/13002/MoinMoin/apps/frontend/vie...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/11584044/diff/13002/MoinMoin/apps/frontend/vie...
MoinMoin/apps/frontend/views.py:80: return redirect(url_for_item(item_name))
guess we need more than one such route, right?
like one per root_mapping entry?

https://codereview.appspot.com/11584044/diff/13002/MoinMoin/apps/misc/views.py
File MoinMoin/apps/misc/views.py (right):

https://codereview.appspot.com/11584044/diff/13002/MoinMoin/apps/misc/views.p...
MoinMoin/apps/misc/views.py:40: root_item = app.cfg.root_mapping[u'']
same here, one such entry per namespace-root.

https://codereview.appspot.com/11584044/diff/13002/MoinMoin/config/default.py
File MoinMoin/config/default.py (right):

https://codereview.appspot.com/11584044/diff/13002/MoinMoin/config/default.py...
MoinMoin/config/default.py:26: from MoinMoin.constants.namespaces import
NAMESPACE_DEFAULT
maybe you should use this constant more often instead of ''.

https://codereview.appspot.com/11584044/diff/13002/MoinMoin/config/default.py...
MoinMoin/config/default.py:464: ('root_mapping', {NAMESPACE_DEFAULT: u'Home'},
"mapping of namespaces to item_roots."),
why do you need the NAMESPACE_DEFAULT: u'Home' entry if you have the behaviour
as documented in line 463?

https://codereview.appspot.com/11584044/diff/13002/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

https://codereview.appspot.com/11584044/diff/13002/MoinMoin/items/__init__.py...
MoinMoin/items/__init__.py:300: fqname = CompositeName(fqname.namespace,
NAME_EXACT, app.cfg.root_mapping.get(fqname.namespace, app.cfg.default_root))
think about whether it maybe make sense to put that code into the CompositeName
class somehow and generally call it (and not use mapping['']).

[ashu1461, 2013-08-03 17:48:28]

https://codereview.appspot.com/11584044/diff/13002/MoinMoin/apps/frontend/vie...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/11584044/diff/13002/MoinMoin/apps/frontend/vie...
MoinMoin/apps/frontend/views.py:80: return redirect(url_for_item(item_name))
On 2013/07/30 21:57:52, Thomas.J.Waldmann wrote:
> guess we need more than one such route, right?
> like one per root_mapping entry?

This is just for the root(needs to be handled separately). rest of them get
redirected in the Item.create func.

[Thomas.J.Waldmann, 2013-08-04 12:20:08]

https://codereview.appspot.com/11584044/diff/13002/MoinMoin/apps/frontend/vie...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/11584044/diff/13002/MoinMoin/apps/frontend/vie...
MoinMoin/apps/frontend/views.py:80: return redirect(url_for_item(item_name))
as both cases are following the same rules, can you do it at one place?

[Thomas.J.Waldmann, 2013-08-05 17:30:21]

https://codereview.appspot.com/11584044/diff/40001/MoinMoin/apps/frontend/vie...
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/11584044/diff/40001/MoinMoin/apps/frontend/vie...
MoinMoin/apps/frontend/views.py:79: item_name =
app.cfg.root_mapping[NAMESPACE_DEFAULT]
this constant is u'' right? see my other comments.

https://codereview.appspot.com/11584044/diff/40001/MoinMoin/apps/misc/views.py
File MoinMoin/apps/misc/views.py (right):

https://codereview.appspot.com/11584044/diff/40001/MoinMoin/apps/misc/views.p...
MoinMoin/apps/misc/views.py:44: revs = list(flaskg.storage.search(q=query))
why do you need list() here?

https://codereview.appspot.com/11584044/diff/40001/MoinMoin/apps/misc/views.p...
MoinMoin/apps/misc/views.py:45: for rev in revs:
you could just merge like:

for rev in flaskg.storage.search(...):

https://codereview.appspot.com/11584044/diff/40001/MoinMoin/themes/__init__.py
File MoinMoin/themes/__init__.py (right):

https://codereview.appspot.com/11584044/diff/40001/MoinMoin/themes/__init__.p...
MoinMoin/themes/__init__.py:231: args['item_name'] = app.cfg.root_mapping[u'']
use the constant?