Moin IncludeWithVals plugins

Moin IncludeWithVals plugins

[ReimarBauer, 2013-01-26 22:13:56]

some comments, first pass through

https://codereview.appspot.com/7220043/diff/1/COPYING
File COPYING (right):

https://codereview.appspot.com/7220043/diff/1/COPYING#newcode1
COPYING:1: GNU GENERAL PUBLIC LICENSE
Be carefull because moins library is based on GPL V2+

not only 3

In a GPL 2 only environment this makes some difficulties.

https://codereview.appspot.com/7220043/diff/1/action/savedict.py
File action/savedict.py (right):

https://codereview.appspot.com/7220043/diff/1/action/savedict.py#newcode26
action/savedict.py:26: page.send_page(send_special=True)
why do you need send_special=True ?

The UI usually uses the message bar for showing errors etc.

https://codereview.appspot.com/7220043/diff/1/action/savedict.py#newcode34
action/savedict.py:34: raise ValueError("You don't have enough rights on this
page")
use a msg on the UI for this too.

The message is likly also translatable then

https://codereview.appspot.com/7220043/diff/1/action/savedict.py#newcode42
action/savedict.py:42: savedict[k] = request.form.get(k)
wikiutil.escape ?

https://codereview.appspot.com/7220043/diff/1/action/savedict.py#newcode46
action/savedict.py:46: savetext = savetext + u' ' + k + u':: ' + savedict[k] +
u'\n'
you can use %s or named.

https://codereview.appspot.com/7220043/diff/1/action/savedict.py#newcode67
action/savedict.py:67: request.theme.add_msg(unicode(msg), "error")
isn't the message in utf-8 encoding? where does it come from?

https://codereview.appspot.com/7220043/diff/1/macro/IncludeVal.py
File macro/IncludeVal.py (right):

https://codereview.appspot.com/7220043/diff/1/macro/IncludeVal.py#newcode13
macro/IncludeVal.py:13: _sysmsg = '<p><strong class="%s">%s</strong></p>'
u ...

https://codereview.appspot.com/7220043/diff/1/macro/IncludeWithVals.py
File macro/IncludeWithVals.py (right):

https://codereview.appspot.com/7220043/diff/1/macro/IncludeWithVals.py#newcode1
macro/IncludeWithVals.py:1: # -*- coding: utf-8 -*-
if there are no special chars used it can be also coding: iso-8859-1

that makes it still easier for some platforms

https://codereview.appspot.com/7220043/diff/1/macro/IncludeWithVals.py#newcode13
macro/IncludeWithVals.py:13: _sysmsg = '<p><strong class="%s">%s</strong></p>'
u' ...

https://codereview.appspot.com/7220043/diff/1/macro/IncludeWithVals.py#newcode33
macro/IncludeWithVals.py:33: (request.script_root, this_page,
request.ReqVars['vardict'])
look how textcha and a ticket is implemented

may be add it similiar to your form. 

You could avoid \" by using """...""" or '...'

[gordon.messmer, 2013-03-03 16:49:19]

Apparently I left these comments in Draft status, and did not send them. :(

https://codereview.appspot.com/7220043/diff/1/action/savedict.py
File action/savedict.py (right):

https://codereview.appspot.com/7220043/diff/1/action/savedict.py#newcode26
action/savedict.py:26: page.send_page(send_special=True)
I don't know what it does, specifically.  That code came directly from
action/edit.py:execute

https://codereview.appspot.com/7220043/diff/1/action/savedict.py#newcode34
action/savedict.py:34: raise ValueError("You don't have enough rights on this
page")
This code matches the same test in macro/__init__.py:macro_GetVal

https://codereview.appspot.com/7220043/diff/1/action/savedict.py#newcode42
action/savedict.py:42: savedict[k] = request.form.get(k)
The values are escaped on display.  I'm pretty sure that escaping them here
would double-escape them.

https://codereview.appspot.com/7220043/diff/1/action/savedict.py#newcode46
action/savedict.py:46: savetext = savetext + u' ' + k + u':: ' + savedict[k] +
u'\n'
Concatenation is faster in cpython.  According to PEP8, though, I should
probably convert this to ''.join()

https://codereview.appspot.com/7220043/diff/1/action/savedict.py#newcode67
action/savedict.py:67: request.theme.add_msg(unicode(msg), "error")
Again, I'm not sure.  This code is also identical to the same function in
action/edit.py:execute

https://codereview.appspot.com/7220043/diff/1/macro/IncludeVal.py
File macro/IncludeVal.py (right):

https://codereview.appspot.com/7220043/diff/1/macro/IncludeVal.py#newcode13
macro/IncludeVal.py:13: _sysmsg = '<p><strong class="%s">%s</strong></p>'
Sounds right.  This code matches macro/Include.py, where sysmsg is also
specified as a non-unicode string.