search + history

search + history

[Reimar Bauer, 2012-08-09 21:45:05]

small comment

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

http://codereview.appspot.com/6459066/diff/4001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1014: pass
with the doc string you don't need pass

http://codereview.appspot.com/6459066/diff/4001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1039: item['branch'] = rev.rev.real_branch
rev.rev sounds a bit strange for the parameter

[breton, 2012-08-09 22:30:16]

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

http://codereview.appspot.com/6459066/diff/4001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1039: item['branch'] = rev.rev.real_branch
On 2012/08/09 21:45:05, Reimar Bauer wrote:
> rev.rev sounds a bit strange for the parameter 

that's done because `rev` is a ProtectedRevision instance; and it has an
attribute `rev`, which is an indexing.Revision instance, where information about
branch is set.

[Reimar Bauer, 2012-08-10 09:15:21]

some comment

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

http://codereview.appspot.com/6459066/diff/4001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1039: item['branch'] = rev.rev.real_branch
On 2012/08/09 22:30:16, breton wrote:
> On 2012/08/09 21:45:05, Reimar Bauer wrote:
> > rev.rev sounds a bit strange for the parameter 
> 
> that's done because `rev` is a ProtectedRevision instance; and it has an
> attribute `rev`, which is an indexing.Revision instance, where information
about
> branch is set.

ok, on long term i think it could be better to know by the variable if we have a
protected revision or not. But that may be needs refactoring on other places
too. So ignore.

http://codereview.appspot.com/6459066/diff/4001/MoinMoin/templates/history.html
File MoinMoin/templates/history.html (right):

http://codereview.appspot.com/6459066/diff/4001/MoinMoin/templates/history.ht...
MoinMoin/templates/history.html:41: <td class="moin-wordbreak">{{ doc.branch
}}</td>
please check if you have to change the foobar template too

[Reimar Bauer, 2012-08-11 09:10:39]

some comments

be careful with py2.7 only features. You could backport such a module if it is
important.

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

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:26: pass
this won't work in py 2.6.0

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:397: # 1. Get branch docs by name
in general

this method becomes longer and longer. On some working state it should become
modularized.

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/search/__init__.py
File MoinMoin/search/__init__.py (right):

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/search/__init__.py#n...
MoinMoin/search/__init__.py:20: SEARCH_ALL_CURRENT: u'All users\' current work',
\' ?

[ThomasJWaldmann, 2012-08-11 11:36:05]

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

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:26: pass
and IF you do something like this, you shouldn't just "pass" there, but do
something alternative, like setting Counter = None (if it is optional, so you
can check for "Counter is None" later) or doing some other import.

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:372: elif type == SEARCH_BRANCHES_HEADS:
rather name this search_type. type() is a builtin.

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:379: # XXX: do we need to search in tags?
why should we?

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:383: # branches and should be removed after the
release
is this workaround still needed?

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:397: # 1. Get branch docs by name
and also it should be made in a way so it is testable.

it is complex enough to consider it broken as long as it is not tested.

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:419: revids_to_follow.append(revid)
you are aware that you are adding/appending a LIST here?

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:424: # 4. Remove all revs which are parents for
>1 revs
not sure if that makes sense. if there is a fork in history, you won't know what
the "always existing" branch was and what the "new" branch was at the time when
it was forked.
so, for the history before the fork point, you don't know which branch it
belongs to. it could very well belong to the branch you are currently searching,
so you should not get rid of it, right?

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1073: Goes through history and sets a branch for
every revision.
why saying that twice?

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1079: if r['branch'] and r['branch_type'] ==
BRANCH]
use constants

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

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/constants/keys.py#ne...
MoinMoin/constants/keys.py:80: SEARCH_ALL = '6'  # all revs
if they are numbers, int would be more elegant. you could also use real strings,
though.

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

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:219: if arg in ['branch', 'branch_type',
'is_userhead', 'revid']:
in case you use the same names here as you have as constants, use the constants.

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/search/__init__.py
File MoinMoin/search/__init__.py (right):

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/search/__init__.py#n...
MoinMoin/search/__init__.py:20: SEARCH_ALL_CURRENT: u'All users\' current work',
On 2012/08/11 09:10:39, Reimar Bauer wrote:
> \' ?

as i already mentioned to xiaq: usually for english, "use this form, it's the
best one due to apostrophes."

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/search/__init__.py#n...
MoinMoin/search/__init__.py:21: SEARCH_BRANCH_HEAD: u'Specific branch\'s head',
A ...

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/search/__init__.py#n...
MoinMoin/search/__init__.py:24: SEARCH_ALL: u'All revision',
revisions

[breton, 2012-08-11 13:07:34]

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

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:26: pass
On 2012/08/11 11:36:05, ThomasJWaldmann wrote:
> and IF you do something like this, you shouldn't just "pass" there, but do
> something alternative, like setting Counter = None (if it is optional, so you
> can check for "Counter is None" later) or doing some other import.

Counter is used in one place only, which is now inactive. But you are right, I
will fix that.

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:372: elif type == SEARCH_BRANCHES_HEADS:
On 2012/08/11 11:36:05, ThomasJWaldmann wrote:
> rather name this search_type. type() is a builtin.

type() is not used anywhere in that function, that's why I named it this way. It
is named so in other places, I guess I chould fix it there too.

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:383: # branches and should be removed after the
release
On 2012/08/11 11:36:05, ThomasJWaldmann wrote:
> is this workaround still needed?

For me only. It exists because there is an index without BRANCH_SRC, which left
from an old index. I will remove it a bit later, after writing tests.

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:397: # 1. Get branch docs by name
On 2012/08/11 11:36:05, ThomasJWaldmann wrote:
> and also it should be made in a way so it is testable.
> 
> it is complex enough to consider it broken as long as it is not tested.

tests in the next patchset

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:419: revids_to_follow.append(revid)
On 2012/08/11 11:36:05, ThomasJWaldmann wrote:
> you are aware that you are adding/appending a LIST here?

No, I am adding/appending a unicode.
ridrf = revids_to_follow.pop()
parentids = all_revs[ridtf].get(PARENTID, [])
for revid in parentids:
    add/append(revid)

Though I should probably use .update here.

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:424: # 4. Remove all revs which are parents for
>1 revs
On 2012/08/11 11:36:05, ThomasJWaldmann wrote:
> not sure if that makes sense. if there is a fork in history, you won't know
what
> the "always existing" branch was and what the "new" branch was at the time
when
> it was forked.
> so, for the history before the fork point, you don't know which branch it
> belongs to. it could very well belong to the branch you are currently
searching,
> so you should not get rid of it, right?

That's why it is left unfinished and should not be ran :)
Maybe I should comment the whole thing out? Or raise NotImplemented here? This
type of search is not shown in the dropdown list on +search.

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1079: if r['branch'] and r['branch_type'] ==
BRANCH]
On 2012/08/11 11:36:05, ThomasJWaldmann wrote:
> use constants

I intentionally didn't use constants here to avoid mixing up with branches and
userheads. `revs` in no way connected to either of branches or userheads.

These k:v are set lower (1108) and are set specially for this function.

If you still think constants must be used, please reply, I'll change it.

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

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/constants/keys.py#ne...
MoinMoin/constants/keys.py:80: SEARCH_ALL = '6'  # all revs
On 2012/08/11 11:36:05, ThomasJWaldmann wrote:
> if they are numbers, int would be more elegant. you could also use real
strings,
> though.

First I wanted to use ints here, but it seems that `gen.option` in templates
requires option values to be strings. I tried to do something with str(), but no
luck. 

Using here something else will also make GET string is browser longer.

Change it to something human-readable anyway?

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

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:219: if arg in ['branch', 'branch_type',
'is_userhead', 'revid']:
On 2012/08/11 11:36:05, ThomasJWaldmann wrote:
> in case you use the same names here as you have as constants, use the
constants.

nope, not the same. These have nothing to do with meta.

[ThomasJWaldmann, 2012-08-12 14:19:14]

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

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:26: pass
> Counter is used in one place only, which is now inactive. > But you are right,
I will fix that.

If you remove the inactive code, do not forget to remove the import also.

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:419: revids_to_follow.append(revid)
ok, ignore my comment, didn't read correctly.

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:424: # 4. Remove all revs which are parents for
>1 revs
if you can't define some sense / some usecase: remove it.

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/apps/frontend/views....
MoinMoin/apps/frontend/views.py:1079: if r['branch'] and r['branch_type'] ==
BRANCH]
use constants. if they are not the same, use new constants. and make sure you
name / document them well enough so one immediately sees the difference.

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

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/constants/keys.py#ne...
MoinMoin/constants/keys.py:80: SEARCH_ALL = '6'  # all revs
> Change it to something human-readable anyway?

Yes, that sounds good.

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

http://codereview.appspot.com/6459066/diff/9001/MoinMoin/items/__init__.py#ne...
MoinMoin/items/__init__.py:219: if arg in ['branch', 'branch_type',
'is_userhead', 'revid']:
 
> nope, not the same. These have nothing to do with meta.

OK, see above: make new constants.

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/_tests/te...
File MoinMoin/apps/frontend/_tests/test_search.py (right):

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/_tests/te...
MoinMoin/apps/frontend/_tests/test_search.py:21: def
test_search_my_current(self):
as I already requested twice at least, I want you to document/comment
non-trivial tests and say what they do.

the name of this one is pretty misleading as it suggests to test something
related to search, but it has nothing to do with search.

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/_tests/te...
MoinMoin/apps/frontend/_tests/test_search.py:37: assert f(search_type) ==
[rev2.revid]
f ?

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/_tests/te...
MoinMoin/apps/frontend/_tests/test_search.py:40: "create some revisions with
different flaskg.user.itemid"
use """...""" for docstrings

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/_tests/te...
MoinMoin/apps/frontend/_tests/test_search.py:43: pass
instead of using "pass", you can just use a docstring.

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

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:225: """updates list1 with values from list2
comparing dict keys
if possible, try to semantically name stuff.

"list" is a data type, but as a name, it doesn't tell much.

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:256: # 6. Get all revids from (already updated)
master branches
somehow 2..6 does not make sense to me, can you make it more precise /
explaining?

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:264: q = {BRANCH_TYPE: TAG}
line 251+ does not talk about tags

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:285: i = all_branches_names.index(term)
this looks a bit inefficient. use dicts?

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:304: # 6. Get all revids from (already updated)
master branches
see above, 2..6 is unclear

and quite some of the code below looks like copy&paste from above and duplicate.

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:347: # 1. Get branches by name
get all branches named <branch>
(should be rather branchname btw)

maybe could be SEARCH_ONE_BRANCH_HEAD ?

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:370: elif search_type == SEARCH_BRANCHES_HEADS:
maybe could be SEARCH_ALL_BRANCHES_HEADS

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:382: # ----
i expect that you remove this temporary workaround crap asap.
the code is hard enough to understand anyway and duplicate even without that.

http://codereview.appspot.com/6459066/diff/8/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:398: # 4. Using all revids build a history for
branch-heads
unclear

[ThomasJWaldmann, 2012-08-18 17:20:27]

http://codereview.appspot.com/6459066/diff/17001/MoinMoin/search/_tests/test_...
File MoinMoin/search/_tests/test_search_helpers.py (right):

http://codereview.appspot.com/6459066/diff/17001/MoinMoin/search/_tests/test_...
MoinMoin/search/_tests/test_search_helpers.py:126: assert f(search_type) is None
ok, comments for tests getting better, but as i already mentioned: please use
complete sentences, not just fragments.