Metadata editor idea. This is just a dirty sketch that shows how I want to make metaeditor work.

Metadata editor idea. This is just a dirty sketch that shows how I want to make metaeditor work.

[Reimar Bauer, 2012-07-08 09:40:13]

some comments

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

http://codereview.appspot.com/6356070/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:662: class ModifyJsonMetaForm(Form):
ModifyMetaForm

as it is json, the name could be maybe shorter

http://codereview.appspot.com/6356070/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:663: meta_text =
String.using(optional=False).with_properties(placeholder=L_("MetaData
(JSON)")).validated_by(ValidJSON())
not a real problem

but somehow that looks ugly because it is a very long line and by that not good
readable. Also I have the impression that this kind of call may become repeated
several times. Somehow similar. May be better do it with a helper function.

http://codereview.appspot.com/6356070/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:678: self[key] = item.meta[key]
what is the reason that 'summary' and 'ptime' is not in item.meta?
If it is always required for blog entries what do you do in the case it is not
there?
And what happens to the other meta information? Is it wanted to skip it?
Because than the method name is misleading it did a selection.

If it is always there, this can be improved

[spy, 2012-07-08 10:25:33]

some explanations

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

http://codereview.appspot.com/6356070/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:663: meta_text =
String.using(optional=False).with_properties(placeholder=L_("MetaData
(JSON)")).validated_by(ValidJSON())
On 2012/07/08 09:40:13, Reimar Bauer wrote:
> not a real problem
> 
> but somehow that looks ugly because it is a very long line and by that not
good
> readable. Also I have the impression that this kind of call may become
repeated
> several times. Somehow similar. May be better do it with a helper function.

This would be changed using Flatland widgets (after xiaq's successful pull of
course).

http://codereview.appspot.com/6356070/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:678: self[key] = item.meta[key]
On 2012/07/08 09:40:13, Reimar Bauer wrote:
> what is the reason that 'summary' and 'ptime' is not in item.meta?
> If it is always required for blog entries what do you do in the case it is not
> there?
> And what happens to the other meta information? Is it wanted to skip it?
> Because than the method name is misleading it did a selection.
> 
> If it is always there, this can be improved

'ptime' and 'summary' are not required fields for a blog entry. Summary is used
as a title of an entry. If there is no summary item.name is used insted. ptime
is also optional, if there is no ptime this entry will not be published and
nothing more.

Also I will add 'tags' field here.

What other metadata for an ordinary wiki item may need to change directly? I
guess meta like 'externallinks', 'itemlinks', 'itemtransclusions', 'parentid',
'wikiname' shouldn't be editable manually, am I right?

http://codereview.appspot.com/6356070/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:729: class ModifyItemForm(self.ModifyForm,
modify_meta_form):
So, is it a good approach when I create an empty class with a dynamically
selected superclass in order to unite two forms?

[Reimar Bauer, 2012-07-08 10:46:49]

answered

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

http://codereview.appspot.com/6356070/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:678: self[key] = item.meta[key]
On 2012/07/08 10:25:34, spy wrote:
> On 2012/07/08 09:40:13, Reimar Bauer wrote:
> > what is the reason that 'summary' and 'ptime' is not in item.meta?
> > If it is always required for blog entries what do you do in the case it is
not
> > there?
> > And what happens to the other meta information? Is it wanted to skip it?
> > Because than the method name is misleading it did a selection.
> > 
> > If it is always there, this can be improved
> 
> 'ptime' and 'summary' are not required fields for a blog entry. Summary is
used
> as a title of an entry. If there is no summary item.name is used insted. ptime
> is also optional, if there is no ptime this entry will not be published and
> nothing more.
> 
> Also I will add 'tags' field here.
> 
> What other metadata for an ordinary wiki item may need to change directly? 

There can be user added meta data, from the builtin moin's metadata it is may be
acl settings per item. 

I
> guess meta like 'externallinks', 'itemlinks', 'itemtransclusions', 'parentid',
> 'wikiname' shouldn't be editable manually, am I right?

yes, I already thought about to make it more visible which are created on the
fly and remain readonly therefore.

http://codereview.appspot.com/6356070/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:729: class ModifyItemForm(self.ModifyForm,
modify_meta_form):
On 2012/07/08 10:25:34, spy wrote:
> So, is it a good approach when I create an empty class with a dynamically
> selected superclass in order to unite two forms?

not sure, if it is the best location to do it. but principle for now ok. Somehow
I think we should look for a better location sooner or later. Because that kind
of factory can be used for any view_method.

[Reimar Bauer, 2012-07-10 07:41:09]

some comments

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

http://codereview.appspot.com/6356070/diff/5001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:655: view_method_keys = [u'', BLOG_ENTRY, ]
just added

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

http://codereview.appspot.com/6356070/diff/5001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:659: print item.meta
use logging for such kind of messages ;)

http://codereview.appspot.com/6356070/diff/5001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:778: print "from_request"
use logging instead

[ThomasJWaldmann, 2012-07-11 13:12:25]

some first comments, please also ask your mentor for review.

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

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:636: VIEW_METHOD, BLOG_ENTRY
new code should rather import from the constants module.

the config imports are mostly old code and just kept for compatibility right
now.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:638: class ModifyMetaForm(Form):
as a general comment: if working in this area, make sure you do not create
conflicting stuff with MaikuMori.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:653: class BaseModifyMetaForm(Form):
when comparing with ModifyMetaForm, calling this one "Base..." seems a bit
strange, as it is not the base of the other one.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:672: self[key] = item.meta[key]
hmm, isn't there a method to fill flatland forms from an object / dict, etc.?

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:674: tags = item.meta[TAGS]
replace these with:
item.meta.get(TAGS, [])

(you don't need that "if" then)

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:683: meta[TAGS] = sorted(set(tags))
maybe such stuff should be combined with validation somehow.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:724: from MoinMoin.apps.frontend.views import
CommentForm
long term, we should get rid of the specialisation of "comment". it is just
metadata as a lot of other stuff, too.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:771: pass # TODO implement other view methods
does this mean nothing else will work right now?

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:1398: pass
rather use a docstring than "pass", it tells more and works also.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/templates/modify_ap...
File MoinMoin/templates/modify_applet.html (right):

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/templates/modify_ap...
MoinMoin/templates/modify_applet.html:24: {% endblock %}
not sure, but maybe the blog editing template should just inherit from the main
editing template one and overwrite the meta block.

the python code could render the blog editing template then, not the applet
editing template.

[spy, 2012-07-12 12:14:55]

some replies and fixes

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

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:636: VIEW_METHOD, BLOG_ENTRY
On 2012/07/11 13:12:25, ThomasJWaldmann wrote:
> new code should rather import from the constants module.
> 
> the config imports are mostly old code and just kept for compatibility right
> now.

Done.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:638: class ModifyMetaForm(Form):
On 2012/07/11 13:12:25, ThomasJWaldmann wrote:
> as a general comment: if working in this area, make sure you do not create
> conflicting stuff with MaikuMori.

Maybe you wanted to say with xiaq. In fact there is a little conflict already
here because I divided ModifyForm into two separate forms.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:653: class BaseModifyMetaForm(Form):
On 2012/07/11 13:12:25, ThomasJWaldmann wrote:
> when comparing with ModifyMetaForm, calling this one "Base..." seems a bit
> strange, as it is not the base of the other one.

ModifyMetaForm is the old JSON modify form. It is not used anymore in my code.
So, should I leave it and somehow make it usable?

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:672: self[key] = item.meta[key]
On 2012/07/11 13:12:25, ThomasJWaldmann wrote:
> hmm, isn't there a method to fill flatland forms from an object / dict, etc.?

existed, done.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:674: tags = item.meta[TAGS]
On 2012/07/11 13:12:25, ThomasJWaldmann wrote:
> replace these with:
> item.meta.get(TAGS, [])
> 
> (you don't need that "if" then)

Done.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:683: meta[TAGS] = sorted(set(tags))
On 2012/07/11 13:12:25, ThomasJWaldmann wrote:
> maybe such stuff should be combined with validation somehow.

Does it make sense to talk to the user that he/she wrote a duplicate tag or a
blank one when tapped two commas in a row?

btw, livejournal.com has a tag string-field exactly the same as I wrote here. It
doesn't use any alerts on errors in tags.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:771: pass # TODO implement other view methods
On 2012/07/11 13:12:25, ThomasJWaldmann wrote:
> does this mean nothing else will work right now?

Not quite. Line 765 defines default meta form.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:1398: pass
On 2012/07/11 13:12:25, ThomasJWaldmann wrote:
> rather use a docstring than "pass", it tells more and works also.

Done.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/templates/modify_ap...
File MoinMoin/templates/modify_applet.html (right):

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/templates/modify_ap...
MoinMoin/templates/modify_applet.html:24: {% endblock %}
On 2012/07/11 13:12:25, ThomasJWaldmann wrote:
> not sure, but maybe the blog editing template should just inherit from the
main
> editing template one and overwrite the meta block.
> 
> the python code could render the blog editing template then, not the applet
> editing template.

in this case I should also choose which template to render according to
VIEW_METHOD in do_modify method  (MoinMoin/items/__init__.py Line 798). Will it
be better?

[Reimar Bauer, 2012-07-13 06:34:13]

there was an older comment I missed to send

A new question:

Can we transclude a blog entry on a normal wiki page?
I am currently not sure if that is wanted or not.

If that is possible Do we want that is there shown a blog_view or normal view.
Or do we want that it can be 
controlled similiar to highlight feature?

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

http://codereview.appspot.com/6356070/diff/11002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:711: pass
I would expect also some logging values for the error case

[spy, 2012-07-13 07:12:16]

On 2012/07/13 06:34:13, Reimar Bauer wrote:
> there was an older comment I missed to send
> 
> A new question:
> 
> Can we transclude a blog entry on a normal wiki page?
> I am currently not sure if that is wanted or not.
> 
> If that is possible Do we want that is there shown a blog_view or normal view.
> Or do we want that it can be 
> controlled similiar to highlight feature?
> 
> http://codereview.appspot.com/6356070/diff/11002/MoinMoin/items/__init__.py
> File MoinMoin/items/__init__.py (right):
> 
>
http://codereview.appspot.com/6356070/diff/11002/MoinMoin/items/__init__.py#n...
> MoinMoin/items/__init__.py:711: pass
> I would expect also some logging values for the error case

Yes, we can transclude a blog entry on a normar wiki page because they all are
normal wiki pages too.

Currently views selection is controlled manually, but we introduce VIEW_METHOD
meta that will determine which of view to show by default.

[ThomasJWaldmann, 2012-07-14 14:47:37]

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

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:638: class ModifyMetaForm(Form):
yes, i meant xiaq.

if you create conflicts, you both (and mentors) will have to agree on something,
so better agree on such stuff early.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:653: class BaseModifyMetaForm(Form):
Until metadata editor is flexible enough to do anything we need, yes, we might
still need the json stuff. maybe it could be part of metadata editor for
anything not yet supported?

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:683: meta[TAGS] = sorted(set(tags))
I didn't specifically refer to tags, but rather to all user input. and catching
errors early and giving the form back to the user with a reasonable error msg is
better than storing crap into storage. it can also do "normalization" there,
like stripping.

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/templates/modify_ap...
File MoinMoin/templates/modify_applet.html (right):

http://codereview.appspot.com/6356070/diff/13002/MoinMoin/templates/modify_ap...
MoinMoin/templates/modify_applet.html:24: {% endblock %}
see my comment there - as form classes and templates using them must correspond,
maybe such a dict where both are referred to makes more sense than having
another big if/else here.

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

http://codereview.appspot.com/6356070/diff/3004/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:675: self[TAGS] = u', '.join(tags)
maybe move that some lines upwards and do meta[TAGS] = ..., so set_flat also
deals with it.

http://codereview.appspot.com/6356070/diff/3004/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:771: pass # TODO implement other view methods
maybe do this with a dict lookup rather than if/else.

key is view_method, value is (FormClass, TemplateFilename)

[xiaq, 2012-07-18 14:23:59]

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

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:312: }
Instead of patching into show_item, we might want to have one more registry for
that in future (like the registry for contenttype).

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/forms.py#newcode75
MoinMoin/forms.py:75: Tags = (Text.using(label=L_('Tags'), optional=True)
Just use flatland.JoinedString, see my metadata editor patch set
(http://codereview.appspot.com/6429043).

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/forms.py#newcode99
MoinMoin/forms.py:99: class BaseModifyMetaForm(Form):
I'll see how i can merge this with my xxxMetaForm implementations...

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

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:637: class ModifyDataForm(BaseChangeForm):
The separation of ModifyMetaForm and ModifyDataForm looks like a good one.

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/themes/modernized/s...
File MoinMoin/themes/modernized/static/css/common.css (right):

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/themes/modernized/s...
MoinMoin/themes/modernized/static/css/common.css:189: #f_ptime,
I feel that instead of adding every f_xxx here we can assign a class to the text
widget (say widget-text) and use the class (.widget-text) here.

[Reimar Bauer, 2012-07-18 15:48:46]

some comments

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/forms.py#newcode27
MoinMoin/forms.py:27: from MoinMoin import log
this import of moins logging module should be very early

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/forms.py#newcode31
MoinMoin/forms.py:31: class ValidTags(Validator):
is there no builtin for this?

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/themes/modernized/s...
File MoinMoin/themes/modernized/static/css/common.css (right):

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/themes/modernized/s...
MoinMoin/themes/modernized/static/css/common.css:189: #f_ptime,
On 2012/07/18 14:23:59, xiaq wrote:
> I feel that instead of adding every f_xxx here we can assign a class to the
text
> widget (say widget-text) and use the class (.widget-text) here.

This should be discussed with jaiditya, because we need that in the stylus file.
This css file will be generated from that.

[Reimar Bauer, 2012-07-20 07:02:16]

some comments

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py#newcode98
MoinMoin/forms.py:98: except:
exception case missing

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

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:323: """ kill metadata entries that we set
automatically when saving and empty ones """
is that a docstring or a comment?

[spy, 2012-07-20 20:51:58]

some replies

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/forms.py#newcode27
MoinMoin/forms.py:27: from MoinMoin import log
On 2012/07/18 15:48:46, Reimar Bauer wrote:
> this import of moins logging module should be very early
> 

Done.

http://codereview.appspot.com/6356070/diff/36001/MoinMoin/forms.py#newcode75
MoinMoin/forms.py:75: Tags = (Text.using(label=L_('Tags'), optional=True)
On 2012/07/18 14:23:59, xiaq wrote:
> Just use flatland.JoinedString, see my metadata editor patch set
> (http://codereview.appspot.com/6429043).

Done.

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py#newcode98
MoinMoin/forms.py:98: except:
On 2012/07/20 07:02:16, Reimar Bauer wrote:
> exception case missing

Done.

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

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:323: """ kill metadata entries that we set
automatically when saving and empty ones """
On 2012/07/20 07:02:16, Reimar Bauer wrote:
> is that a docstring or a comment?

This is a docstring. Is something wrong with it?

[Reimar Bauer, 2012-07-21 07:28:23]

answered

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

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:323: """ kill metadata entries that we set
automatically when saving and empty ones """
On 2012/07/20 20:51:58, spy wrote:
> On 2012/07/20 07:02:16, Reimar Bauer wrote:
> > is that a docstring or a comment?
> 
> This is a docstring. Is something wrong with it?

Because of the meta_"filter" name, I assumed not reading from killing.

It is more like meta_"cleanup"

[ThomasJWaldmann, 2012-07-22 11:20:25]

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py#newcode6
MoinMoin/forms.py:6: MoinMoin - Flatland widgets and modify metadata forms
either "forms for metadata modification"
or "metadata modification forms"
or "metadata editing forms"

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py#newcode16
MoinMoin/forms.py:16: import simplejson as json
we require python 2.6 for moin2, json is included there

did you see such code at another place in moin2 or why did you do it that way?

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py#newcode81
MoinMoin/forms.py:81: .validated_by(Converted(incorrect=L_("Please use the
following format: YYYY-MM-DD HH:MM:SS"))))
for some american people, it might be useful to hint at 24h clock. you somehow
indirectly do, as there is no mention of AM/PM, but maybe we should do that more
directly.

maybe by example? if we would compute the current date, we could just show that
as placeholder + "23:59:59"

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py#newcode100
MoinMoin/forms.py:100: return True
don't we have a json validator already at some other place?
did you copy it?

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

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:704: return (ModifyForm, meta_form[1])
parens needed?

[spy, 2012-07-22 13:19:40]

answered

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py#newcode6
MoinMoin/forms.py:6: MoinMoin - Flatland widgets and modify metadata forms
On 2012/07/22 11:20:25, ThomasJWaldmann wrote:
> either "forms for metadata modification"
> or "metadata modification forms"
> or "metadata editing forms"

Done.

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py#newcode16
MoinMoin/forms.py:16: import simplejson as json
On 2012/07/22 11:20:25, ThomasJWaldmann wrote:
> we require python 2.6 for moin2, json is included there
> 
> did you see such code at another place in moin2 or why did you do it that way?

this code has been moved from items/__init__/.py
ok, replaced by "import json" only.

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py#newcode81
MoinMoin/forms.py:81: .validated_by(Converted(incorrect=L_("Please use the
following format: YYYY-MM-DD HH:MM:SS"))))
On 2012/07/22 11:20:25, ThomasJWaldmann wrote:
> for some american people, it might be useful to hint at 24h clock. you somehow
> indirectly do, as there is no mention of AM/PM, but maybe we should do that
more
> directly.
> 
> maybe by example? if we would compute the current date, we could just show
that
> as placeholder + "23:59:59"

Done.

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py#newcode100
MoinMoin/forms.py:100: return True
On 2012/07/22 11:20:25, ThomasJWaldmann wrote:
> don't we have a json validator already at some other place?
> did you copy it?

it has been moved from items/__init__.py since I want to bring together all
metaeditor stuff here.

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

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:323: """ kill metadata entries that we set
automatically when saving and empty ones """
On 2012/07/21 07:28:23, Reimar Bauer wrote:
> On 2012/07/20 20:51:58, spy wrote:
> > On 2012/07/20 07:02:16, Reimar Bauer wrote:
> > > is that a docstring or a comment?
> > 
> > This is a docstring. Is something wrong with it?
> 
> Because of the meta_"filter" name, I assumed not reading from killing.
> 
> It is more like meta_"cleanup"
> 

Done.

[Reimar Bauer, 2012-07-23 12:24:27]

so far now, have to look in details later

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

http://codereview.appspot.com/6356070/diff/42002/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:501: item_name = item.name,
no blanks surrounding of keywords =

[spy, 2012-07-23 15:56:27]

done

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

http://codereview.appspot.com/6356070/diff/42002/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:501: item_name = item.name,
On 2012/07/23 12:24:27, Reimar Bauer wrote:
> no blanks surrounding of keywords =

Done.

[ThomasJWaldmann, 2012-07-28 23:01:43]

found some issues, please fix.

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py#newcode98
MoinMoin/forms.py:98: except:
ehrm, except: is correct here (maybe there should be a comment pointing out that
we want to catch ANY exception that happens due to unserializing).

So please revert that change and maybe add that comment.

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

http://codereview.appspot.com/6356070/diff/42002/MoinMoin/constants/keys.py#n...
MoinMoin/constants/keys.py:95: BLOG_ENTRY = u"blog_entry"
please merge main repo and read the comment at top of keys.py and fix all
constants accordingly.

[Reimar Bauer, 2012-07-30 13:29:18]

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py
File MoinMoin/forms.py (right):

http://codereview.appspot.com/6356070/diff/26011/MoinMoin/forms.py#newcode16
MoinMoin/forms.py:16: import simplejson as json
On 2012/07/22 11:20:25, ThomasJWaldmann wrote:
> we require python 2.6 for moin2, json is included there
> 
> did you see such code at another place in moin2 or why did you do it that way?

My fault, we have it like that on other places. Just search for simplejson