userheads

userheads

[eSyr, 2012-07-20 12:51:16]

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/backends/stores.py
File MoinMoin/storage/backends/stores.py (right):

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/backends/stores...
MoinMoin/storage/backends/stores.py:145: def retrieve_branch(self, userheadid):
retrieve_userhead?

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

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:855: if branchname.startswith('$') and
len(branchname) == len(make_uuid()) + 1:
check against \$[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12} maybe?

[ThomasJWaldmann, 2012-07-20 13:14:29]

well, just some low-level comments.

basically I don't know exactly what you are implementing PRECISELY. if there are
some uptodate and cleaned up docs about that, point me to them.

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

http://codereview.appspot.com/6423063/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:191: if rev_id and rev_id != CURRENT:  # is not
current
how is that comment helpful?

http://codereview.appspot.com/6423063/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:196: rev = item.get_revision(CURRENT)
somehow this if/elif/else looks like it could be simplified.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/backends/stores.py
File MoinMoin/storage/backends/stores.py (right):

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/backends/stores...
MoinMoin/storage/backends/stores.py:146: return self._get_userhead(userheadid)
at what daytime did you add that?

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

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:520: self._modify_branches_index(index,
BRANCHES, self.wikiname,
you give BRANCHES to schema?

that's a little strange as you use BRANCHES for the indexname also.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:525: index = open_dir(index_dir,
indexname=BRANCHES)
same indexname again?

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:771: userhead =
self._document(idx_name=USERHEADS, userid=user.itemid)
this is not the userhead, but an indexed document.
you get the userhead in the following line out of that document:

(please avoid confusing naming)

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:855: if branchname.startswith('$') and
len(branchname) == len(make_uuid()) + 1:
IIRC we have a constant for this length, so you do not need to compute
len(make_uuid()) over and over again.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:862: return branch[BRANCH_REVID]
see comment for line 771 - this is an indexed document, not a branch.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:863: # no branch retrieved from index,
checking default
the question here is whether we should just fall back silently to the master
branch or maybe rather raise some exception.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:869: # still no branch; using the first
found
see above.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:966: branch = u'master'
sometimes you use 'master', sometimes u'master'.

i suggest your use a constant MASTER_BRANCH or so to get this consistent
everywhere.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:974: branch_state[BRANCH_TYPE] =
u'branch'
same here.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:982: # userheads
maybe try to avoid pointless comments.
putting comments is fine, but they should say more than what's obvious anyway.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:990: userhead_state[UH_USER] =
flaskg.user
this is the complete User object, i doubt you want to index that.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:1031: # get current user's branchname
see above

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:1034: branchname = 'master'
maybe do that in get_users_branch()?

[Reimar Bauer, 2012-07-20 14:16:33]

some comments

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

http://codereview.appspot.com/6423063/diff/1/MoinMoin/items/__init__.py#oldco...
MoinMoin/items/__init__.py:514: action=unicode(action),
why is unicode() needed?

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

http://codereview.appspot.com/6423063/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:467: def _save(self, meta, data=None,
branch='master', name=None,
type unicode not str

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

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:425: mode='add', procs=1, limitmb=256):
unicode type

[breton, 2012-07-21 02:28:21]

On 2012/07/20 13:14:29, ThomasJWaldmann wrote:
> well, just some low-level comments.
> 
> basically I don't know exactly what you are implementing PRECISELY. if there
are
> some uptodate and cleaned up docs about that, point me to them.

Generally I am implementing the sequence of actions described on 75-89 of
http://etherpad.osuosl.org/moin2-bms

[breton, 2012-07-21 02:28:57]

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

http://codereview.appspot.com/6423063/diff/1/MoinMoin/items/__init__.py#oldco...
MoinMoin/items/__init__.py:514: action=unicode(action),
On 2012/07/20 14:16:34, Reimar Bauer wrote:
> why is unicode() needed?

http://idunnolol.jpg.to/

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

http://codereview.appspot.com/6423063/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:191: if rev_id and rev_id != CURRENT:  # is not
current
On 2012/07/20 13:14:29, ThomasJWaldmann wrote:
> how is that comment helpful?

Err, changed the condition, added comment, returned the condition, forgot to
remove the comment.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:196: rev = item.get_revision(CURRENT)
On 2012/07/20 13:14:29, ThomasJWaldmann wrote:
> somehow this if/elif/else looks like it could be simplified.

if a specific revid is explicitly given, show it.
else if a branchname is given, show it
else (nothing is given) show current revision (whatever current means).
/me doesn't see how it can be simplified.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:467: def _save(self, meta, data=None,
branch='master', name=None,
On 2012/07/20 14:16:34, Reimar Bauer wrote:
> type unicode not str

thanks, fixed.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/backends/stores.py
File MoinMoin/storage/backends/stores.py (right):

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/backends/stores...
MoinMoin/storage/backends/stores.py:145: def retrieve_branch(self, userheadid):
On 2012/07/20 12:51:16, eSyr wrote:
> retrieve_userhead?

right, fixed.

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

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:520: self._modify_branches_index(index,
BRANCHES, self.wikiname,
On 2012/07/20 13:14:29, ThomasJWaldmann wrote:
> you give BRANCHES to schema?
> 
> that's a little strange as you use BRANCHES for the indexname also.

right, there should be self.schemas[BRANCHES]. // should return to cycle on the
left side :(

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:525: index = open_dir(index_dir,
indexname=BRANCHES)
On 2012/07/20 13:14:29, ThomasJWaldmann wrote:
> same indexname again?

thanks, fixed. Should definetely return back to the cycle on the left

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:771: userhead =
self._document(idx_name=USERHEADS, userid=user.itemid)
On 2012/07/20 13:14:29, ThomasJWaldmann wrote:
> this is not the userhead, but an indexed document.
> you get the userhead in the following line out of that document:
> 
> (please avoid confusing naming)

in fact, that's exactly the userhead, as defined on 51 of the EP. On the next
line is a branchname or a $-prefixed revid is returned, which is a string.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:855: if branchname.startswith('$') and
len(branchname) == len(make_uuid()) + 1:
On 2012/07/20 12:51:16, eSyr wrote:
> check against \$[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12} maybe?

I don't think it'll be very helpful. Also, sane people wouldn't call a branch
with a (8 + 4*3 + 12)-len name, so this check should be enough.

Also, you sure this will never be changed?

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:855: if branchname.startswith('$') and
len(branchname) == len(make_uuid()) + 1:
On 2012/07/20 13:14:29, ThomasJWaldmann wrote:
> IIRC we have a constant for this length, so you do not need to compute
> len(make_uuid()) over and over again.

fixed

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:863: # no branch retrieved from index,
checking default
On 2012/07/20 13:14:29, ThomasJWaldmann wrote:
> the question here is whether we should just fall back silently to the master
> branch or maybe rather raise some exception.

as we decided on EP:83

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:966: branch = u'master'
On 2012/07/20 13:14:29, ThomasJWaldmann wrote:
> sometimes you use 'master', sometimes u'master'.
> 
> i suggest your use a constant MASTER_BRANCH or so to get this consistent
> everywhere.

Agreed. Add it to constats.keys?

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:982: # userheads
On 2012/07/20 13:14:29, ThomasJWaldmann wrote:
> maybe try to avoid pointless comments.
> putting comments is fine, but they should say more than what's obvious anyway.

just dividing the into sections

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:990: userhead_state[UH_USER] =
flaskg.user
On 2012/07/20 13:14:29, ThomasJWaldmann wrote:
> this is the complete User object, i doubt you want to index that.

thanks, fixed.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:1034: branchname = 'master'
On 2012/07/20 13:14:29, ThomasJWaldmann wrote:
> maybe do that in get_users_branch()?

in fact, I don't see any reason to do it. Removed.

[breton, 2012-07-21 09:58:15]

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

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:771: userhead =
self._document(idx_name=USERHEADS, userid=user.itemid)
Also, I forgot to use itemid here. I think this function belongs to class
indexing.Item rather than to indexing.IndexingMiddleware

[ThomasJWaldmann, 2012-07-22 10:45:28]

some answers.

btw: unit tests?

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

http://codereview.appspot.com/6423063/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:196: rev = item.get_revision(CURRENT)
hmmm, right. i thought the first and third part can be merged, but no. so just
keep it that way.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:467: def _save(self, meta, data=None,
branch='master', name=None,

> thanks, fixed.

if you have fixed all the stuff, upload another state to same codereview id.

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

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:855: if branchname.startswith('$') and
len(branchname) == len(make_uuid()) + 1:
well, maybe a function get_uuid(s) would be good, that either returns None (no
uuid found) or the uuid.

that internally can then use any reasonable check to determine wheter we have a
uuid and strip whatever prefix there is.

http://codereview.appspot.com/6423063/diff/1/MoinMoin/storage/middleware/inde...
MoinMoin/storage/middleware/indexing.py:966: branch = u'master'
yes.

[ThomasJWaldmann, 2012-07-31 09:28:41]

as an additional general comment: you are giving MASTER_BRANCH and ALL_REVS as
param values rather often, maybe those should be named args with that as the
default.

http://codereview.appspot.com/6423063/diff/9001/MoinMoin/storage/middleware/_...
File MoinMoin/storage/middleware/_tests/test_indexing.py (right):

http://codereview.appspot.com/6423063/diff/9001/MoinMoin/storage/middleware/_...
MoinMoin/storage/middleware/_tests/test_indexing.py:69: def
test_nonmaster_branch(self):
what precisely is this test testing?

http://codereview.appspot.com/6423063/diff/9001/MoinMoin/storage/middleware/_...
MoinMoin/storage/middleware/_tests/test_indexing.py:200: assert
expected_rev.revid == rev.revid  # it is really the latest one
this test is ok, but it would even work if branching did not work at all.

what you need to add is getting the branch1 head and checking whether that is as
expected also.

http://codereview.appspot.com/6423063/diff/9001/MoinMoin/storage/middleware/i...
File MoinMoin/storage/middleware/indexing.py (right):

http://codereview.appspot.com/6423063/diff/9001/MoinMoin/storage/middleware/i...
MoinMoin/storage/middleware/indexing.py:895: assert False
?

http://codereview.appspot.com/6423063/diff/9001/MoinMoin/storage/middleware/i...
MoinMoin/storage/middleware/indexing.py:1015: """
as there is one userhead PER ITEM, i think some usage of self.itemid is missing
here.

[ThomasJWaldmann, 2012-08-07 20:29:50]

some comments.

found your new patchset late as you didn't answer any comment of the last one.
thus you only find ne patchset when manually comparing date of last own comment
to date of latest patchset...

http://codereview.appspot.com/6423063/diff/9001/MoinMoin/storage/middleware/_...
File MoinMoin/storage/middleware/_tests/test_indexing.py (right):

http://codereview.appspot.com/6423063/diff/9001/MoinMoin/storage/middleware/_...
MoinMoin/storage/middleware/_tests/test_indexing.py:69: def
test_nonmaster_branch(self):
On 2012/07/31 09:28:41, ThomasJWaldmann wrote:
> what precisely is this test testing?

no comment?

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

http://codereview.appspot.com/6423063/diff/14004/MoinMoin/constants/keys.py#n...
MoinMoin/constants/keys.py:63: UH_ITEMID = BRANCH_ITEMID = "itemid"
why not just using ITEMID then, if it is the same string anyway (and also the
same meaning)?

http://codereview.appspot.com/6423063/diff/14004/MoinMoin/storage/middleware/...
File MoinMoin/storage/middleware/_tests/test_indexing.py (right):

http://codereview.appspot.com/6423063/diff/14004/MoinMoin/storage/middleware/...
MoinMoin/storage/middleware/_tests/test_indexing.py:402: def
test_current_user_get(self):
for non-trivial tests add a short docstring and described what exactly they
test.

http://codereview.appspot.com/6423063/diff/14004/MoinMoin/storage/middleware/...
MoinMoin/storage/middleware/_tests/test_indexing.py:435: flaskg.user = User()
why is this needed?

http://codereview.appspot.com/6423063/diff/14004/MoinMoin/storage/middleware/...
File MoinMoin/storage/middleware/indexing.py (right):

http://codereview.appspot.com/6423063/diff/14004/MoinMoin/storage/middleware/...
MoinMoin/storage/middleware/indexing.py:715: def generate_branchname(self,
itemid):
what exactly means itemid here?

http://codereview.appspot.com/6423063/diff/14004/MoinMoin/storage/middleware/...
MoinMoin/storage/middleware/indexing.py:723: branch =
self._document(idx_name=BRANCHES, **{BRANCH_DST: itemid})
i don't understand itemid usage here.

http://codereview.appspot.com/6423063/diff/14004/MoinMoin/storage/middleware/...
MoinMoin/storage/middleware/indexing.py:1004: q = {UH_USER: userid, UH_ITEMID:
self.itemid,BRANCH_TYPE: USERHEAD}
a blank missing

http://codereview.appspot.com/6423063/diff/14004/MoinMoin/storage/middleware/...
MoinMoin/storage/middleware/indexing.py:1130: 
in general, code is hard to follow/understand, i suggest you add more docs.