More cleanup on items module.

Closing.

[xiaq, 2012-06-26 16:28:27]

This is actually a follow-up of my previous changeset
(http://hg.moinmo.in/moin/2.0/rev/339668f8ad06). Notes:

* Item methods are refactored. The motivation is that the family of methods -
rename, delete, revert, destroy, modify to be request-agnostic. Previously
modify() looks into the request and take corresponding actions, and the split of
do_modify() and modify() is somehow arbitrary. The new code is cleaner and more
testable.

* As a result, test cases and frontend codes were modified too.

* I ran a py.test and there is no regression (compared to default branch). The
following were manually tested:

 - Creating and modifying MoinWiki items
 - Reverting an item
 - Creating and modifying TWikiDraw, AnyWikiDraw and SvgDraw items
 - Drag-and-drop to upload files in +index view (jfu)

No regression was observed.

* My previous change set broke creating item from template (template data is not
loaded). This patch set fixes it.

* Item template is now supported for Draw items too.

[Reimar Bauer, 2012-06-26 20:21:23]

some comments

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

http://codereview.appspot.com/6330064/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:697: revid, size = item.modify({}, data,
contenttype_guessed=contenttype)
is that {} needed?

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

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#oldco...
MoinMoin/items/__init__.py:703: item = self.item
is self.item removed?

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

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:423: 
no additional line needed

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:707: item = self

Why?

Where is item used?

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/_tests/test_Item.py
File MoinMoin/items/_tests/test_Item.py (right):

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/_tests/test_Item....
MoinMoin/items/_tests/test_Item.py:293: another_data = 'another_test_data'
u'....'

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/_tests/test_Item....
MoinMoin/items/_tests/test_Item.py:294: another_meta = {'another_test_key':
'another_test_value'}
also u'...'

[xiaq, 2012-06-27 03:39:25]

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

http://codereview.appspot.com/6330064/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:697: revid, size = item.modify({}, data,
contenttype_guessed=contenttype)
On 2012/06/26 20:21:23, Reimar Bauer wrote:
> is that {} needed? 

Yes. This makes it explicit that there is no metadata available.

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

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#oldco...
MoinMoin/items/__init__.py:703: item = self.item
On 2012/06/26 20:21:23, Reimar Bauer wrote:
> is self.item removed?

Yes. The idea that forms no longer keeps reference to the item once initialized.

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

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:423: 
On 2012/06/26 20:21:23, Reimar Bauer wrote:
> no additional line needed

ok, fixing

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:707: item = self
On 2012/06/26 20:21:23, Reimar Bauer wrote:
> 
> Why?
> 
> Where is item used?

Below, form = self.ModifyForm.from_item(item)
It reads "item is self by default, unless template_name is specified".

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/_tests/test_Item.py
File MoinMoin/items/_tests/test_Item.py (right):

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/_tests/test_Item....
MoinMoin/items/_tests/test_Item.py:293: another_data = 'another_test_data'
On 2012/06/26 20:21:23, Reimar Bauer wrote:
> u'....'

I noticed this, but other code in test_Item.py use plain str literals. Should
those all be converted to unicode literals?

[Reimar Bauer, 2012-06-27 06:07:54]

answered

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/_tests/test_Item.py
File MoinMoin/items/_tests/test_Item.py (right):

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/_tests/test_Item....
MoinMoin/items/_tests/test_Item.py:293: another_data = 'another_test_data'
On 2012/06/27 03:39:25, xiaq wrote:
> On 2012/06/26 20:21:23, Reimar Bauer wrote:
> > u'....'
> 
> I noticed this, but other code in test_Item.py use plain str literals. Should
> those all be converted to unicode literals?

Because that is ascii in the test text one can argument it is the same one byte
string as declared as unicode data type. But because all our methods handle
unicode strings I suggest to  have also in tests unicode values used. We could
think on input validation (if not already done) for the data type too.

This is needs a separate changeset for fixing it on all places, I file an issue
for that fix.

[ThomasJWaldmann, 2012-06-28 16:41:08]

looks ok, but see the minor comments.

thanks for cleaning up there.

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

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:724: return redirect(url_for_item(self.name))
at some other places that uses this pattern it is (IIRC) like this:

if POST:
   form =
   if form.validate():
       ...
       return ...
# here comes the GET and non-valid POST code
...
return render_template(...)

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:1121: self['data_text'] =
item.data_storage_to_internal(item.data)
this is somehow a bit asymmetric.
here it is storage -> internal used for filling form
below is form -> internal -> storage used for reading the form.

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:1303: # meta_text doesn't work
file a bug please

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:1387: Draw.ModifyForm._load(self, item)
wut??

[xiaq, 2012-06-29 03:56:20]

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

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:724: return redirect(url_for_item(self.name))
On 2012/06/28 16:41:09, ThomasJWaldmann wrote:
> at some other places that uses this pattern it is (IIRC) like this:
> 
> if POST:
>    form =
>    if form.validate():
>        ...
>        return ...
> # here comes the GET and non-valid POST code
> ...
> return render_template(...)

I followed the pattern of +revert, +delete and +destroy. The above pattern only
applies when there is no code specific to GET compared with an erroneous POST.

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:1121: self['data_text'] =
item.data_storage_to_internal(item.data)
On 2012/06/28 16:41:09, ThomasJWaldmann wrote:
> this is somehow a bit asymmetric.
> here it is storage -> internal used for filling form
> below is form -> internal -> storage used for reading the form.

Fixing in next patch set.

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:1303: # meta_text doesn't work
On 2012/06/28 16:41:09, ThomasJWaldmann wrote:
> file a bug please

Filed https://bitbucket.org/thomaswaldmann/moin-2.0/issue/204

http://codereview.appspot.com/6330064/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:1387: Draw.ModifyForm._load(self, item)
On 2012/06/28 16:41:09, ThomasJWaldmann wrote:
> wut??

Fixing...