Index code cleanup

Index code cleanup

[xiaq, 2012-08-08 04:00:11]

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

http://codereview.appspot.com/6457091/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:101: self.meta[NAME] = self.item.name
This used to give DummyRev a NAME metadata property which is utilized in the new
Item.get_flat_index().

http://codereview.appspot.com/6457091/diff/1/MoinMoin/storage/middleware/inde...
File MoinMoin/storage/middleware/indexing.py (right):

http://codereview.appspot.com/6457091/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:231: ITEMTYPE: ID(stored=True),
Changes in this file will be a separate changeset.

[xiaq, 2012-08-10 02:50:52]

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

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:430: def make_flat_index(self, subitems):
ignore delta above this line except for the change of IndexEntry definition.
it's due to rebasing.

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:534: 
ignore delta below this line too...

[ThomasJWaldmann, 2012-08-10 10:14:54]

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

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:134: class IndexEntry(object):
namedtuple?

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:139: self.hassubitem = hassubitem
0 subitems
1 subitem
>=2 subitems

so, except you mean the ==1 case only, i suggest it is called hassubitems.

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:415: 
names are a bit confusing here.

prefix maybe should be subitems_prefix.

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:435: latter having the hassubitem flag set in its
IndexEntry. In case the
their

(and in general, i find this a bit hard to understand)

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:473: :param selected_groups: if set, items whose
contentype belonging to the
"items with contenttype belonging..."

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:476: # Filter for startswith
guess we can live without that comment :)

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:479: # re.match always anchors at the start of the
string
re.match?

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:503: # If an item's contenttype not present in the
default contenttype list,
...is not...

[Reimar Bauer, 2012-08-12 06:55:09]

look at create right for those "new" items

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

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:437: and 'application/x.nonexistent' contenttype is
created.
I wonder currently which parentid one of this not connected subitems had.

wanted_items should list these items too and indicate their state. (can be done
afterwards)


We should look on the create right.

What happens to the limitation after one with that right creates the last
subitem. All parents will exist after that. I guess any user with write rights
can add content, change contenttype etc. If so it should be fixed that create
right is required to change such an item.

There should be a test added.

[ThomasJWaldmann, 2012-08-12 18:18:18]

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

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:437: and 'application/x.nonexistent' contenttype is
created.
On 2012/08/12 06:55:09, Reimar Bauer wrote:
> I wonder currently which parentid one of this not connected subitems had.

PARENTID is related to revisioning ("previous revision") and NOT related to
items and subitems.

[xiaq, 2012-08-13 02:22:23]

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

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:134: class IndexEntry(object):
On 2012/08/10 10:14:54, ThomasJWaldmann wrote:
> namedtuple?

i ditched readonly namedtuple in order to make "flatenning" one-pass, since it's
necessary to modify the hassubitem property after creating an IndexEntry.

There may be a better way...

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:139: self.hassubitem = hassubitem
On 2012/08/10 10:14:54, ThomasJWaldmann wrote:
> 0 subitems
> 1 subitem
> >=2 subitems
> 
> so, except you mean the ==1 case only, i suggest it is called hassubitems.

makes sense.

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:415: 
On 2012/08/10 10:14:54, ThomasJWaldmann wrote:
> names are a bit confusing here.
> 
> prefix maybe should be subitems_prefix.

ok.

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:435: latter having the hassubitem flag set in its
IndexEntry. In case the
On 2012/08/10 10:14:54, ThomasJWaldmann wrote:
> their
> 
> (and in general, i find this a bit hard to understand)

/me reworking this comment..

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:479: # re.match always anchors at the start of the
string
On 2012/08/10 10:14:54, ThomasJWaldmann wrote:
> re.match?

a leftover...

i used `if re.match(startswith, e.relname, re.I)` as filtering condition, but
dropped that later after deciding it's too clever. :)

http://codereview.appspot.com/6457091/diff/4001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:503: # If an item's contenttype not present in the
default contenttype list,
On 2012/08/10 10:14:54, ThomasJWaldmann wrote:
> ...is not...

a mistake inherited from old code...

seems this comment is no longer relevant. removing.

[xiaq, 2012-08-16 01:42:37]

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

http://codereview.appspot.com/6457091/diff/1008/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:362:
@frontend.route('/+show/<itemname:item_name>')
ignore delta above this line.

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

http://codereview.appspot.com/6457091/diff/1008/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:138: class IndexEntry(object):
ignore delta above this line

http://codereview.appspot.com/6457091/diff/1008/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:151: def create(cls, name=u'', itemtype=None,
contenttype=None, rev_id=CURRENT, item=None):
ignore delta in this method

http://codereview.appspot.com/6457091/diff/1008/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:557: )
ignore delta below this line

[ThomasJWaldmann, 2012-08-16 08:05:34]

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

http://codereview.appspot.com/6457091/diff/1008/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:348: # The second form only accpets GET since
modifying a historical revision is not allowed (yet).
typo: accepts

and remove the " (yet)". we don't know whether we will allow basing edits on
historical revisions soon. and strictly taken, we never modify revisions (we
just create new revisions based on them).

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

http://codereview.appspot.com/6457091/diff/1008/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:438: foo, a dummy IndexEntry is manufactured for the
latter, with
"created" instead of "manufactured"?

[xiaq, 2012-08-17 09:09:12]

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

http://codereview.appspot.com/6457091/diff/1008/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:348: # The second form only accpets GET since
modifying a historical revision is not allowed (yet).
On 2012/08/16 08:05:35, ThomasJWaldmann wrote:
> typo: accepts
> 
> and remove the " (yet)". we don't know whether we will allow basing edits on
> historical revisions soon. and strictly taken, we never modify revisions (we
> just create new revisions based on them).

ok, will put the fix in another changeset.

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

http://codereview.appspot.com/6457091/diff/1008/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:438: foo, a dummy IndexEntry is manufactured for the
latter, with
On 2012/08/16 08:05:35, ThomasJWaldmann wrote:
> "created" instead of "manufactured"?

ok.

[ThomasJWaldmann, 2012-08-17 17:53:03]

11:11  xiaq$ ThomasWaldmann: http://codereview.appspot.com/6457091/ (index
cleanup) updated. the only geniune change from patch set 3 is 
             "manufactured" -> "created"...

Somehow I do not see that change, but other changes...

[xiaq, 2012-08-18 05:46:05]

On 2012/08/17 17:53:03, ThomasJWaldmann wrote:
> 11:11  xiaq$ ThomasWaldmann: http://codereview.appspot.com/6457091/ (index
> cleanup) updated. the only geniune change from patch set 3 is 
>              "manufactured" -> "created"...
> 
> Somehow I do not see that change, but other changes...

patch set 4 against 3,
http://codereview.appspot.com/6457091/diff2/1008:6002/MoinMoin/items/__init__.py
line 440 on right hand side.

patch set 5 has more changes.

[ThomasJWaldmann, 2012-08-19 14:09:00]

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

http://codereview.appspot.com/6457091/diff/5014/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:759: selected_groups.remove(u'submit')
why was this removed?

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

http://codereview.appspot.com/6457091/diff/4016/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:283: prefixlen = len(self.subitems_prefix)
cosmetic: move this 1 line lower as it belongs to that code.

also:

old_prefixlen = ...
new_prefix = name + '/'
...
    child_newname = new_prefix + ...

http://codereview.appspot.com/6457091/diff/4016/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:430: def make_flat_index(self, subitems):
btw, is there a test for this method?

http://codereview.appspot.com/6457091/diff/4016/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:456: # Find the *direct* subitem that is the ancestor
of current
"ancestor" is somehow sounding like it has something to do either with time or
with a sequence/order.

i think you might rather mean "parent" (vs. "child") which is a term used when
discussing tree-like stuff and hierarchies.

http://codereview.appspot.com/6457091/diff/4016/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:462: # NOTE Patching the index when encountering a
disconnected
NOTE:

http://codereview.appspot.com/6457091/diff/4016/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:463: # subitem might break the ordering. eg. suppose
the global
e.g.

http://codereview.appspot.com/6457091/diff/4016/MoinMoin/items/_tests/test_It...
File MoinMoin/items/_tests/test_Item.py (right):

http://codereview.appspot.com/6457091/diff/4016/MoinMoin/items/_tests/test_It...
MoinMoin/items/_tests/test_Item.py:30: index.append(IndexEntry(relname,
Item.create('/'.join((basename, relname))).meta, hassubitem))
if this is not going to get longer / more complex, it could be a list
comprehension.

[xiaq, 2012-08-19 14:33:29]

I've fixed the trivial things in patch set 10.

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

http://codereview.appspot.com/6457091/diff/4016/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:430: def make_flat_index(self, subitems):
On 2012/08/19 14:09:01, ThomasJWaldmann wrote:
> btw, is there a test for this method?

it's now tested altogether with get_index. i'll put up a test case...

http://codereview.appspot.com/6457091/diff/4016/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:456: # Find the *direct* subitem that is the ancestor
of current
On 2012/08/19 14:09:01, ThomasJWaldmann wrote:
> "ancestor" is somehow sounding like it has something to do either with time or
> with a sequence/order.
> 

I learned this from my textbook :) "Ancestor" is not that uncommon:
http://en.wikipedia.org/wiki/Tree_%28data_structure%29#Terminology

> i think you might rather mean "parent" (vs. "child") which is a term used when
> discussing tree-like stuff and hierarchies.

I was looking for a word that expresses the "(the parent node of )+current node"
(consider text within double quotes a regexp), and "ancestor" came to my mind.
That was what I learned in my textbook after all, the Wiki may be a little wrong
in stating "parent" and "ancestor" are synonyms.

what about "(maybe indirect) parent"?

[xiaq, 2012-08-21 18:00:19]

I modified TestItem.testIndex to test for make_flat_index and filter_index
separately. (question: Should the trivial helper get_index be tested? Methinks
it's so trivial that the probability it should go wrong is slighter than the
probability the test cases go wrong :-)

There is also a trivial comment change.

[ThomasJWaldmann, 2012-08-21 19:02:57]

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

http://codereview.appspot.com/6457091/diff/4016/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:456: # Find the *direct* subitem that is the ancestor
of current
hmm, when thinking about it again, i think you can leave it as is. maybe add an
example.

[ThomasJWaldmann, 2012-08-22 14:10:06]

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

http://codereview.appspot.com/6457091/diff/16001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:462: # are its ancestors.
i didn't mean an example for what an ancestor is, but rather for what subitems
your are finding here. giving an example for that, you can have a much shorter,
much clearer comment.