Implementing experimental itemtype

[ThomasJWaldmann, 2012-07-22 12:32:41]

some feedback, could not look at everything yet (and at some places i was
assuming you mostly moved the code, not changing it)

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

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:17: Each class correspond to an itemtype value.
corresponds

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:179: created according to the item's itemtype
metadatum, and the
"metadata entry" rather than "metadatum" maybe?

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:186: itemtype = u'null'
if you really need a str, 'none' maybe?

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:237: item_templates =
self.content.get_templates(self.contenttype)
hmm, should this really be a method of content object?

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:291: def _write_stream(self, content, new_rev,
bufsize=8192):
move to content class?

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:466: data = data.encode(charset) # XXX wrong! if
contenttype gives a coding, we MUST use THAT.
where is charset defined?

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:608: class BaseItem(Item):
deriving something called BaseXXX from XXX is a bit strange.

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:655: item_registry.register(Default._factory,
u'null')
do you mean NonExistent._factory?
'none' here too (or even use 'nonexistent'?)

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py
File MoinMoin/items/content.py (right):

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py#newcode14
MoinMoin/items/content.py:14: contenttype value.
maybe somehow mention content == data, vs. metadata.

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py#newcode61
MoinMoin/items/content.py:61: NAME, NAME_EXACT, WIKINAME, CONTENTTYPE, SIZE,
TAGS, HASH_ALGORITHM
if you create a new module, maybe rather import from constants.XXX

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py#newcod...
MoinMoin/items/content.py:244: class NullContent(BaseContent):
NonExisting rather than NullContent?

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py#newcod...
MoinMoin/items/content.py:245: pass
rather use a docstring than "pass".

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py#newcod...
MoinMoin/items/content.py:504: # no PIL, we can't do anything, we just output
the revision data as is
if PIL is None:

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py#newcod...
MoinMoin/items/content.py:1023: content_registry.register(SvgDraw._factory,
Type('application/x-svgdraw'))
btw, as you are moving around lots of code now anyway (which makes it hard to
review), you could also split into:
base stuff
text
images / drawings
audio / video
other
(or any other reasonable split you come up with)

[xiaq, 2012-07-22 15:59:59]

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

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:17: Each class correspond to an itemtype value.
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> corresponds

fixing

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:179: created according to the item's itemtype
metadatum, and the
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> "metadata entry" rather than "metadatum" maybe?

ok.

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:186: itemtype = u'null'
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> if you really need a str, 'none' maybe?

ok

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:237: item_templates =
self.content.get_templates(self.contenttype)
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> hmm, should this really be a method of content object?

no, just a dirty fix. fixing a bit later.

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:291: def _write_stream(self, content, new_rev,
bufsize=8192):
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> move to content class?

ok.

but grepping for _write_stream reveals that it is not used anywhere, and i guess
it's just some leftover. remove it?

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:466: data = data.encode(charset) # XXX wrong! if
contenttype gives a coding, we MUST use THAT.
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> where is charset defined?

it was in MoinMoin.config, but i change the import to

from MoinMoin.constants.contenttypes import charset, CONTENTTYPE_GROUPS

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:608: class BaseItem(Item):
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> deriving something called BaseXXX from XXX is a bit strange.

indeed. :) merging it back into Item.

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:655: item_registry.register(Default._factory,
u'null')
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> do you mean NonExistent._factory?
> 'none' here too (or even use 'nonexistent'?)

fixing. 'nonexistent' looks better.

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py
File MoinMoin/items/content.py (right):

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py#newcode14
MoinMoin/items/content.py:14: contenttype value.
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> maybe somehow mention content == data, vs. metadata.

ok

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py#newcode61
MoinMoin/items/content.py:61: NAME, NAME_EXACT, WIKINAME, CONTENTTYPE, SIZE,
TAGS, HASH_ALGORITHM
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> if you create a new module, maybe rather import from constants.XXX

was blindly copied from items/__init__.py, cleaning up.

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py#newcod...
MoinMoin/items/content.py:244: class NullContent(BaseContent):
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> NonExisting rather than NullContent?

i'd rather name it NonExistentContent to agree with contenttype (and avoid
confusion with items.NonExistent).

also i'm thinking about just assigning item.content = None to mean "no content"
rather than inventing a contenttype.

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py#newcod...
MoinMoin/items/content.py:245: pass
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> rather use a docstring than "pass".

fixing.

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py#newcod...
MoinMoin/items/content.py:504: # no PIL, we can't do anything, we just output
the revision data as is
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> if PIL is None:

was copied from original items/__init__.py. maybe fix it in another changeset
for sanity's sake?

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/content.py#newcod...
MoinMoin/items/content.py:1023: content_registry.register(SvgDraw._factory,
Type('application/x-svgdraw'))
On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> btw, as you are moving around lots of code now anyway (which makes it hard to
> review), you could also split into:
> base stuff
> text
> images / drawings
> audio / video
> other
> (or any other reasonable split you come up with)

instead of doing more splitting, what about keeping everything in
items/__init__.py when restructuring (which is easy to review) and do a "move
things around"-only changeset (which is less likely to break things)?

[Reimar Bauer, 2012-07-23 06:55:08]

some comments

I try a practical test

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

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:466: data = data.encode(charset) # XXX wrong! if
contenttype gives a coding, we MUST use THAT.
On 2012/07/22 15:59:59, xiaq wrote:
> On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> > where is charset defined?
> 
> it was in MoinMoin.config, but i change the import to
> 
> from MoinMoin.constants.contenttypes import charset, CONTENTTYPE_GROUPS

hmm, in the past we needed it in config because we hadn't utf-8 but iso-xxxx-x
or cpc or whatever
There it was with one config option possible to setup the encoding the editor
uses.

Having it in contenttypes only makes it impossible to modify it for one wiki
instance.

So is utf-8 now a constant or did someone want to change it?

http://codereview.appspot.com/6432058/diff/5003/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6432058/diff/5003/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:786: def internal_representation(self,
converters=['smiley']):
This is the smiley convertor usage?

While internal of that method explicitly smiley_conv is used
the converters variable sounds like it could be variable

[xiaq, 2012-07-23 15:34:05]

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

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:466: data = data.encode(charset) # XXX wrong! if
contenttype gives a coding, we MUST use THAT.
On 2012/07/23 06:55:08, Reimar Bauer wrote:
> On 2012/07/22 15:59:59, xiaq wrote:
> > On 2012/07/22 12:32:42, ThomasJWaldmann wrote:
> > > where is charset defined?
> > 
> > it was in MoinMoin.config, but i change the import to
> > 
> > from MoinMoin.constants.contenttypes import charset, CONTENTTYPE_GROUPS
> 
> hmm, in the past we needed it in config because we hadn't utf-8 but iso-xxxx-x
> or cpc or whatever
> There it was with one config option possible to setup the encoding the editor
> uses.
> 
> Having it in contenttypes only makes it impossible to modify it for one wiki
> instance.
> 
> So is utf-8 now a constant or did someone want to change it?

Citing config/__init__.py:

# provide compatibility for stuff not using MoinMoin.constants yet

Judging from the content of config/__init__.py (it consists of a series of
imports only) it seems that is the sole purpose of the it now. So I followed
Thomas's suggestion and changed the import from MoinMoin.config to
MoinMoin.constants.contenttypes.

BTW, user configuration is kept in the app.config variable, rather than in the
config module.

http://codereview.appspot.com/6432058/diff/5003/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6432058/diff/5003/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:786: def internal_representation(self,
converters=['smiley']):
On 2012/07/23 06:55:08, Reimar Bauer wrote:
> This is the smiley convertor usage?
> 
> While internal of that method explicitly smiley_conv is used
> the converters variable sounds like it could be variable
> 

didn't change the code, just moved around.

[ThomasJWaldmann, 2012-07-23 16:51:44]

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

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:291: def _write_stream(self, content, new_rev,
bufsize=8192):
oh, oops? doublecheck / remove it.

http://codereview.appspot.com/6432058/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:466: data = data.encode(charset) # XXX wrong! if
contenttype gives a coding, we MUST use THAT.
charset is wrong anyway, see that comment "XXX wrong! if contenttype gives a
codi ng, we MUST use THAT.". not required to fix that NOW, though, this
changeset is already large enough and this issue isn't new either.

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:57: #from MoinMoin.items.content import
ROWS_DATA
remove it at the end

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:63: #from .content import content_registry
remove it at the end

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:167: return cls(name, **kw)
what is this needed for?

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:553: 
did you change anything of the stuff above?
hard to review if lots of stuff gets moved around...

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:561: class Contentful(Item):
wut? :) sounds strange somehow.

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:563: Base class for Item subclasses that have a
content.
... have content.

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:601: # XXX
is that XXX from you? means what?

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:1195: # no PIL, we can't do anything, we just output
the revision data as is
I already commented on this, but you lost it by moving stuff around...

You already checked the PIL import, so use "if PIL is None:".

Also please re-check if you lost other feedback while moving stuff here.

[xiaq, 2012-07-23 17:16:51]

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:57: #from MoinMoin.items.content import
ROWS_DATA
On 2012/07/23 16:51:44, ThomasJWaldmann wrote:
> remove it at the end

After separating contenttype codes __init__.py this line will be uncommented,
and the ROWS_DATA import on the previous line be removed.

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:63: #from .content import content_registry
On 2012/07/23 16:51:44, ThomasJWaldmann wrote:
> remove it at the end

It is needed after splitting the file into __init__.py and content.py. So I'll
remove the leading # at the end. :)

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:167: return cls(name, **kw)
On 2012/07/23 16:51:44, ThomasJWaldmann wrote:
> what is this needed for?

For item_registry.

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:553: 
On 2012/07/23 16:51:44, ThomasJWaldmann wrote:
> did you change anything of the stuff above?

almost nothing, except for some necessities to keep things working (eg.
self._render_data -> self.content._render.data).

> hard to review if lots of stuff gets moved around...

yeah, sorry :(

but this is basically a "move-things-around" patch set, i try not to add new
stuff. maybe codereview needs a more intelligent diffing algorithm that can
follow code moving :-P (btw, seriously, is there a way to make it easier to
review while moving code around?)

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:561: class Contentful(Item):
On 2012/07/23 16:51:44, ThomasJWaldmann wrote:
> wut? :) sounds strange somehow.

Yeah i couldn't resist to name it so :-P

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:563: Base class for Item subclasses that have a
content.
On 2012/07/23 16:51:44, ThomasJWaldmann wrote:
> ... have content.

fixing

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:601: # XXX
On 2012/07/23 16:51:44, ThomasJWaldmann wrote:
> is that XXX from you? means what?

means "the same XXX as the previous XXX"...

http://codereview.appspot.com/6432058/diff/1013/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:1195: # no PIL, we can't do anything, we just output
the revision data as is
On 2012/07/23 16:51:44, ThomasJWaldmann wrote:
> I already commented on this, but you lost it by moving stuff around...
> 
> You already checked the PIL import, so use "if PIL is None:".
> 
> Also please re-check if you lost other feedback while moving stuff here.

(after double checking) nothing apart from the PIL comment.

btw, after putting content.py back to __init__.py the above line is actually not
in the patch anymore.

[Reimar Bauer, 2012-07-23 21:40:57]

ok

I think next I want to see that tests cover all those changes
It is difficult to hold back other refactoring ideas while reading the moved
around code.

http://codereview.appspot.com/6432058/diff/26/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6432058/diff/26/MoinMoin/items/__init__.py#newc...
MoinMoin/items/__init__.py:9: 
sometimes we have itemtype and sometimes item_type

if it is the same meaning, please use same writing

sometimes the default for itemtype is None and sometimes it is 'default'. Can it
both times None?

some other parameters have also that writting issue:
content_type or contenttype

http://codereview.appspot.com/6432058/diff/26/MoinMoin/items/__init__.py#newc...
MoinMoin/items/__init__.py:66: COLS = 80
constant?

http://codereview.appspot.com/6432058/diff/26/MoinMoin/items/__init__.py#newc...
MoinMoin/items/__init__.py:67: ROWS_META = 10
constants?

http://codereview.appspot.com/6432058/diff/26/MoinMoin/items/__init__.py#newc...
MoinMoin/items/__init__.py:752: ROWS_DATA = 20
repeating constant

http://codereview.appspot.com/6432058/diff/26/MoinMoin/items/__init__.py#newc...
MoinMoin/items/__init__.py:753: 
both lines sound like regular constants and also may be they should be
changeable in wikiconfig

http://codereview.appspot.com/6432058/diff/26/MoinMoin/items/__init__.py#newc...
MoinMoin/items/__init__.py:836: def internal_representation(self,
converters=['smiley']):
Thomas:

that code looks like limited to smileys. Is that used for any 
other convertor than smiley?

[Reimar Bauer, 2012-07-23 21:46:17]

some more comments

http://codereview.appspot.com/6432058/diff/26/MoinMoin/templates/modify_binar...
File MoinMoin/templates/modify_binary.html (left):

http://codereview.appspot.com/6432058/diff/26/MoinMoin/templates/modify_binar...
MoinMoin/templates/modify_binary.html:6: {{ forms.render_textcha(gen, form) }}
where is the forms.render_textcha ?

http://codereview.appspot.com/6432058/diff/26/MoinMoin/templates/modify_show_...
File MoinMoin/templates/modify_show_itemtype_selection.html (right):

http://codereview.appspot.com/6432058/diff/26/MoinMoin/templates/modify_show_...
MoinMoin/templates/modify_show_itemtype_selection.html:8: {% for itname,
itlabel, itdesc in itemtypes %}
hopefully we don't get that "i" beast on us

http://codereview.appspot.com/6432058/diff/26/MoinMoin/templates/modify_text....
File MoinMoin/templates/modify_text.html (left):

http://codereview.appspot.com/6432058/diff/26/MoinMoin/templates/modify_text....
MoinMoin/templates/modify_text.html:4: {% block subitem_navigation %}
where is that now?

[xiaq, 2012-07-24 02:50:05]

http://codereview.appspot.com/6432058/diff/26/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6432058/diff/26/MoinMoin/items/__init__.py#newc...
MoinMoin/items/__init__.py:9: 
On 2012/07/23 21:40:57, Reimar Bauer wrote:
> sometimes we have itemtype and sometimes item_type
> 
> if it is the same meaning, please use same writing
> 
> sometimes the default for itemtype is None and sometimes it is 'default'. Can
it
> both times None?
> 
> some other parameters have also that writting issue:
> content_type or contenttype 

content_type was inherited from old code, but i shouldn't be following that and
writing item_type :). fixing.

also the motivation for keeping content_type was to keep it out of the diff, but
since it IS in the diff due to code moving, i'd fix that as well.

so to summarize i'll do
:%s/\(content\|item\)_type/\1type/g

http://codereview.appspot.com/6432058/diff/26/MoinMoin/items/__init__.py#newc...
MoinMoin/items/__init__.py:67: ROWS_META = 10
On 2012/07/23 21:40:57, Reimar Bauer wrote:
> constants?

see comments below

http://codereview.appspot.com/6432058/diff/26/MoinMoin/items/__init__.py#newc...
MoinMoin/items/__init__.py:752: ROWS_DATA = 20
On 2012/07/23 21:40:57, Reimar Bauer wrote:
> repeating constant

fixing

http://codereview.appspot.com/6432058/diff/26/MoinMoin/items/__init__.py#newc...
MoinMoin/items/__init__.py:753: 
On 2012/07/23 21:40:57, Reimar Bauer wrote:
> both lines sound like regular constants and also may be they should be
> changeable in wikiconfig

yes but let's do that in another changeset.

http://codereview.appspot.com/6432058/diff/26/MoinMoin/templates/modify_binar...
File MoinMoin/templates/modify_binary.html (left):

http://codereview.appspot.com/6432058/diff/26/MoinMoin/templates/modify_binar...
MoinMoin/templates/modify_binary.html:6: {{ forms.render_textcha(gen, form) }}
On 2012/07/23 21:46:17, Reimar Bauer wrote:
> where is the forms.render_textcha ?

was moved to modify.html.

http://codereview.appspot.com/6432058/diff/26/MoinMoin/templates/modify_show_...
File MoinMoin/templates/modify_show_itemtype_selection.html (right):

http://codereview.appspot.com/6432058/diff/26/MoinMoin/templates/modify_show_...
MoinMoin/templates/modify_show_itemtype_selection.html:8: {% for itname,
itlabel, itdesc in itemtypes %}
On 2012/07/23 21:46:17, Reimar Bauer wrote:
> hopefully we don't get that "i" beast on us

there was "ctname, ctlabel"...
what about just `name, label, desc`?

http://codereview.appspot.com/6432058/diff/26/MoinMoin/templates/modify_text....
File MoinMoin/templates/modify_text.html (left):

http://codereview.appspot.com/6432058/diff/26/MoinMoin/templates/modify_text....
MoinMoin/templates/modify_text.html:4: {% block subitem_navigation %}
On 2012/07/23 21:46:17, Reimar Bauer wrote:
> where is that now?

in modify.html too.

now subitem_navigation was in show.html and modify_text.html, so for showing it
is filled for items of all contenttypes, but for modifying it is only filled for
items of text contenttypes.

i had to fix that since modify_some_contenttype.html templates are no longer
complete pages but only a provider for the extra_head and data_editor macros.

the way to produce a nicer changeset history would be to fix the
subitem_navigation first before applying the itemtype patch set.

[xiaq, 2012-07-24 02:55:03]

new patch set 7, featuring nice comments about code being moved around, please
review! :)

i also rearranged the code a bit to look more similar to the original revision,
therefore you might want to looking at the patch set directly instead of a delta
from a previous patch set (which are more different from the original revision).

btw. issues brought up by dreimark in comments for patch set 5 and 6 hasn't been
addressed, doing that in the next patch set.

[xiaq, 2012-07-24 07:34:31]

Shiny new patch set! :) I tried to include a summary in the patch set title, but
it gets trimmed in the web interface.

* a substitution (item|content)_type -> \1type [1] in items/__init__.py.

* removed template_name from arguments of Item.do_modify. 

  Previously it was filled with the value of `request.values.get('template')` by
the +modify handler and passed to do_modify. This made sense since the creation
of all contenttypes had a template selection step, but with the introduction of
itemtypes it is now up to the Item subclass to decide whether there will be a
template selection step. So passing template_name doesn't always make sense -
it's already useless to NonExistent. Moreover, a specific Item subclass may also
decide to look at other (arbitrary) entries in request.values, in which case
passing template_name only make things asymmetric.

  I thought about introducing the removal of template_name argument in a
follow-up changeset, but with the introduction of itemtypes the do_modify
methods are already being extensively restructured, so I think it's ok to
include it in this changeset.

[xiaq, 2012-07-27 05:22:06]

In patch set 11 i've adapted all test cases so there is no additional failures
introduced. There are also several random cleanups and fixes.

Also the patch set is rebased off latest revision of main repo, which includes
spy's blog codes. The rebase is smooth with no conflicts.

[Reimar Bauer, 2012-07-27 15:44:17]

some comments

Later we should have a review of all used data in tests and also return values.
Often they are not of unicode type.

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/constants/keys.py
File MoinMoin/constants/keys.py (right):

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/constants/keys.py#n...
MoinMoin/constants/keys.py:30: ITEMTYPE = "itemtype"
see

https://bitbucket.org/thomaswaldmann/moin-2.0/issue/208/type-of-keys

new keys in unicode type and others later with a refactoring commit

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:11: MoinMoin - highlevel items
that impresses that we somwhere have lowlevel items

where?

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/items/_tests/test_I...
File MoinMoin/items/_tests/test_Item.py (right):

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/items/_tests/test_I...
MoinMoin/items/_tests/test_Item.py:309: # new items in the storage to set up the
test environment, but its use should

Why do you want it only for the test environment?

If it is replaced with Content.create then the test should also use that or we
test something which won't be used in real practice.

[xiaq, 2012-07-27 16:13:30]

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/constants/keys.py
File MoinMoin/constants/keys.py (right):

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/constants/keys.py#n...
MoinMoin/constants/keys.py:30: ITEMTYPE = "itemtype"
On 2012/07/27 15:44:17, Reimar Bauer wrote:
> see
> 
> https://bitbucket.org/thomaswaldmann/moin-2.0/issue/208/type-of-keys
> 
> new keys in unicode type and others later with a refactoring commit

ok, fixing.

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:11: MoinMoin - highlevel items
On 2012/07/27 15:44:17, Reimar Bauer wrote:
> that impresses that we somwhere have lowlevel items
> 
> where?
> 

various types of storage items in storage/middleware/. what about wording it as
"highlevel (frontend) items"?

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/items/_tests/test_I...
File MoinMoin/items/_tests/test_Item.py (right):

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/items/_tests/test_I...
MoinMoin/items/_tests/test_Item.py:309: # new items in the storage to set up the
test environment, but its use should
On 2012/07/27 15:44:17, Reimar Bauer wrote:
> 
> Why do you want it only for the test environment?
> 

mind the context :)

when testing Content behaviors, item.create is only needed to *set up* the test
environment. subsequent calls should go to content.create. certainly item.create
is also used in various other circumstances, when its use when testing Content
should be restricted.

> If it is replaced with Content.create then the test should also use that or we
> test something which won't be used in real practice.

[Reimar Bauer, 2012-07-27 16:46:20]

ok

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6432058/diff/29005/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:11: MoinMoin - highlevel items
On 2012/07/27 16:13:30, xiaq wrote:
> On 2012/07/27 15:44:17, Reimar Bauer wrote:
> > that impresses that we somwhere have lowlevel items
> > 
> > where?
> > 
> 
> various types of storage items in storage/middleware/. what about wording it
as
> "highlevel (frontend) items"?

or just frontend items

[ThomasJWaldmann, 2012-07-28 00:02:33]

as you referred to here from you pull request, i did a final(?) review of this.
wondering whether all these "for review purposes" comments are still in the pull
request.

i find some more minor things additional to these, though, please fix and create
a new pull request.

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:445: # TODO implement Content.create and use it
Content.create here
sounds strange

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:63: #from .content import content_registry
still needed?

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:151: # XXX Moved verbatim from below Item untouched
{{{
^^ such stuff should be removed before pull request. did you?

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:573: # TODO better name and clearer definition
it won't get better if you do not really document what this shall mean.

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:579: # of itemtype keyword argument plus the comment
above it.
i don't see an itemtype keyword for this method. do you mean the
render_template() call?

did you remove this for the pull request?

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:586: # XXX avoid the magic string
what does this mean exactly?

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:598: # Draw, with modifications.
removed this for pull request?

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:606: itemtype=u'default',
see above.

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:850: # Moved from Item, untouched {{{
removed for pull request?

[xiaq, 2012-07-28 01:41:28]

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:445: # TODO implement Content.create and use it
Content.create here
On 2012/07/28 00:02:33, ThomasJWaldmann wrote:
> sounds strange

fixed (removed the second duplicate Content.create)

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:63: #from .content import content_registry
On 2012/07/28 00:02:33, ThomasJWaldmann wrote:
> still needed?

not now, but needed as soon as i separate Content (and derived classes) into the
content submodule in a followup changeset.

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:151: # XXX Moved verbatim from below Item untouched
{{{
On 2012/07/28 00:02:33, ThomasJWaldmann wrote:
> ^^ such stuff should be removed before pull request. did you?

certainly.

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:579: # of itemtype keyword argument plus the comment
above it.
On 2012/07/28 00:02:33, ThomasJWaldmann wrote:
> i don't see an itemtype keyword for this method. do you mean the
> render_template() call?
> 

yes.

> did you remove this for the pull request?

yes.

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:586: # XXX avoid the magic string
On 2012/07/28 00:02:33, ThomasJWaldmann wrote:
> what does this mean exactly?

changed the comment to "TODO u'default' should be a constant."

it should be a constant in (not existing atm) constants/keys/itemtypes.py, which
will be added in a followup changeset.

http://codereview.appspot.com/6432058/diff/30003/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:598: # Draw, with modifications.
On 2012/07/28 00:02:33, ThomasJWaldmann wrote:
> removed this for pull request?

ah, left out this one. fixing.