WhooshIndex object creation at at start up, implemented some functions for working with indexes

[Reimar Bauer, 2011-07-05 21:22:02]

questions and comments ;)

http://codereview.appspot.com/4650073/diff/1/MoinMoin/app.py
File MoinMoin/app.py (right):

http://codereview.appspot.com/4650073/diff/1/MoinMoin/app.py#newcode175
MoinMoin/app.py:175: index_dir=app.cfg.index_dir
I would prefer that you create a local var first and pass that.

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
File MoinMoin/storage/backends/indexing.py (left):

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
MoinMoin/storage/backends/indexing.py:209: 
empty line ?

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
MoinMoin/storage/backends/indexing.py:294: 
is it meta, because we always use it in singular.

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
MoinMoin/storage/backends/indexing.py:211: # Importing Whoosh stuff
comment neccessary?

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
MoinMoin/storage/backends/indexing.py:258: # Whoosh stuff
well that comment tells what exactly?
if it is such unspecifix - you can remove it

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
MoinMoin/storage/backends/indexing.py:352: found_doc_number =
latest_revs_searcher.document_number(uuid=metas[UUID],
doc_number, without found? because you check later also if it has a val >=0

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
MoinMoin/storage/backends/indexing.py:355: if found_doc_number >= 0:
0? is it -1 if it does no exists?

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
MoinMoin/storage/backends/indexing.py:392: metas["uuid"] = uuid
please name it meta, see above too

[Michael Mayorov, 2011-07-06 02:41:35]

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
File MoinMoin/storage/backends/indexing.py (left):

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
MoinMoin/storage/backends/indexing.py:294: 
On 2011/07/05 21:22:03, Reimar Bauer wrote:
> is it meta, because we always use it in singular.
> 

I try to give names as the same way as in original

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
MoinMoin/storage/backends/indexing.py:355: if found_doc_number >= 0:
On 2011/07/05 21:22:03, Reimar Bauer wrote:
> 0? is it -1 if it does no exists?

0 is first document in index. If document not found, whoosh searcher returns
nothing

[Reimar Bauer, 2011-07-06 07:12:49]

None question, comment

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/1/MoinMoin/storage/backends/indexi...
MoinMoin/storage/backends/indexing.py:355: if found_doc_number >= 0:
On 2011/07/06 02:41:35, Michael Mayorov wrote:
> On 2011/07/05 21:22:03, Reimar Bauer wrote:
> > 0? is it -1 if it does no exists?
> 
> 0 is first document in index. If document not found, whoosh searcher returns
> nothing

it returns None ?
if so write it as
if found_doc_number is not None:

[ThomasWaldmann, 2011-07-06 08:02:04]

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:312: def update_item_whoosh(self, meta):
the other methods are using "metas" also, so please keep "metas".

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:320: async_writer.add_document(**meta)
if doc_number:
   delete...
add...

and wasn't there some update... call just to do that?

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:354: if doc_number >= 0:
from the whoosh docs about document_number():
"this function returns either an int or None."

so, please fix this "if" appropriately.

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:385: def add_rev_whoosh(self, uuid, revno,
meta):
metas

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:431: if doc_number >= 0:
see above

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:491: yield (doc[MTIME], mountpoint +
doc[NAME], doc["rev_no"])
result is not compatible with the other history implementation, you should at
least add a comment about that you left out rev_metas from the tuple.

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:505: with
self.index_object.all_revisions_index.searcher() as all_revs_searcher:
if you compare to the sqla version, you see it uses "item_table", which is
equivalent to your latest_revs_index, not to your all_revs_index.

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:521: with
self.index_object.all_revisions_index.searcher() as all_revs_searcher:
see above

[Michael Mayorov, 2011-07-06 08:41:32]

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:320: async_writer.add_document(**meta)
On 2011/07/06 08:02:04, ThomasWaldmann wrote:
> if doc_number:
>    delete...
> add...
> 
> and wasn't there some update... call just to do that?

update_document just do the same things

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:491: yield (doc[MTIME], mountpoint +
doc[NAME], doc["rev_no"])
On 2011/07/06 08:02:04, ThomasWaldmann wrote:
> result is not compatible with the other history implementation, you should at
> least add a comment about that you left out rev_metas from the tuple.

I removed rev_metas because it isn't using, also I think what doc[MTIME] should
be removed too for same reason

[ThomasWaldmann, 2011-07-06 08:54:09]

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/5001/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:491: yield (doc[MTIME], mountpoint +
doc[NAME], doc["rev_no"])
On 2011/07/06 08:41:32, Michael Mayorov wrote:
> On 2011/07/06 08:02:04, ThomasWaldmann wrote:
> > result is not compatible with the other history implementation, you should
at
> > least add a comment about that you left out rev_metas from the tuple.
> 
> I removed rev_metas because it isn't using, also I think what doc[MTIME]
should
> be removed too for same reason

If you create methods that mirror some existing methods, you use exactly the
same API for them. If you think something needs global changes, add a TODO for
it and give the precise reasons. But you don't create an inconsistent API.

[Reimar Bauer, 2011-07-06 16:02:48]

some comments

http://codereview.appspot.com/4650073/diff/11001/MoinMoin/storage/backends/in...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/11001/MoinMoin/storage/backends/in...
MoinMoin/storage/backends/indexing.py:77: # an item was created with the name
"first" a qnd then renamed to "second."
typo

http://codereview.appspot.com/4650073/diff/11001/MoinMoin/storage/backends/in...
MoinMoin/storage/backends/indexing.py:528: metadata = dict([(str(key), value)
Is this now an other varname for metas ?

[Michael Mayorov, 2011-07-06 16:08:59]

http://codereview.appspot.com/4650073/diff/11001/MoinMoin/storage/backends/in...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/11001/MoinMoin/storage/backends/in...
MoinMoin/storage/backends/indexing.py:528: metadata = dict([(str(key), value)
On 2011/07/06 16:02:48, Reimar Bauer wrote:
> Is this now an other varname for metas ?

No, I don't touch keys and values, but I select only that keys which lie in
schema_fields

[ThomasWaldmann, 2011-07-06 21:25:04]

http://codereview.appspot.com/4650073/diff/7004/MoinMoin/storage/backends/ind...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/7004/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:84: logging.warning("HISTORY: name %s
revno %s" % (name, rev_no))
if you configure logging correctly, you do not need "warning".

http://codereview.appspot.com/4650073/diff/7004/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:429: def remove_rev_whoosh(self, uuid,
revno):
i see you only removing the rev from all_revs index, what about latest_revs
index?

http://codereview.appspot.com/4650073/diff/7004/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:493: yield (doc[MTIME], mountpoint +
doc[NAME], doc["rev_no"])
it is still not compatible with other history method

http://codereview.appspot.com/4650073/diff/7004/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:516: continue
use .get() method of dict instead of try/except.

http://codereview.appspot.com/4650073/diff/7004/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:529: if docs:
what is the value of docs if it doesn't find anything?

do you really need the "if"?

[Michael Mayorov, 2011-07-07 05:12:47]

http://codereview.appspot.com/4650073/diff/7004/MoinMoin/storage/backends/ind...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/7004/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:84: logging.warning("HISTORY: name %s
revno %s" % (name, rev_no))
On 2011/07/06 21:25:04, ThomasWaldmann wrote:
> if you configure logging correctly, you do not need "warning".

I looked at documentation, but didn't found about allowing debug log. I use
Debug=True in my local config, but notghing appears.

[Michael Mayorov, 2011-07-07 05:19:10]

http://codereview.appspot.com/4650073/diff/20001/MoinMoin/storage/backends/in...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/20001/MoinMoin/storage/backends/in...
MoinMoin/storage/backends/indexing.py:457: Yield ready-to-use history raw data
for this backend.
Typo, fixed

[ThomasWaldmann, 2011-07-07 08:38:16]

http://codereview.appspot.com/4650073/diff/20001/MoinMoin/storage/backends/in...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/20001/MoinMoin/storage/backends/in...
MoinMoin/storage/backends/indexing.py:431: with
self.index_object.all_revisions_index.searcher() as all_revs_searcher:
i don't think there is a need to nest the "with" clauses.

please first do latest, then do all afterwards.

http://codereview.appspot.com/4650073/diff/20001/MoinMoin/storage/backends/in...
MoinMoin/storage/backends/indexing.py:457: Yield ready-to-use history raw data
for this backend.
On 2011/07/07 05:19:10, Michael Mayorov wrote:
> Typo, fixed

I don't see a typo been fixed. But you made formatting worse.

http://codereview.appspot.com/4650073/diff/20001/MoinMoin/storage/backends/in...
MoinMoin/storage/backends/indexing.py:498: yield (doc[MTIME], mountpoint +
doc[NAME], doc["rev_no"])
see last review

[ThomasWaldmann, 2011-07-07 09:34:25]

http://codereview.appspot.com/4650073/diff/14002/MoinMoin/script/maint/index.py
File MoinMoin/script/maint/index.py (right):

http://codereview.appspot.com/4650073/diff/14002/MoinMoin/script/maint/index....
MoinMoin/script/maint/index.py:158: 
how about just showing repr(rev)?

[Michael Mayorov, 2011-07-07 13:03:22]

http://codereview.appspot.com/4650073/diff/14002/MoinMoin/script/maint/index.py
File MoinMoin/script/maint/index.py (right):

http://codereview.appspot.com/4650073/diff/14002/MoinMoin/script/maint/index....
MoinMoin/script/maint/index.py:158: 
On 2011/07/07 09:34:25, ThomasWaldmann wrote:
> how about just showing repr(rev)?
> 

If later we'll store content in indexes then output will be quite big

[ThomasWaldmann, 2011-07-07 13:28:54]

http://codereview.appspot.com/4650073/diff/14002/MoinMoin/script/maint/index.py
File MoinMoin/script/maint/index.py (right):

http://codereview.appspot.com/4650073/diff/14002/MoinMoin/script/maint/index....
MoinMoin/script/maint/index.py:158: 
On 2011/07/07 13:03:22, Michael Mayorov wrote:
> On 2011/07/07 09:34:25, ThomasWaldmann wrote:
> > how about just showing repr(rev)?
> > 
> 
> If later we'll store content in indexes then output will be quite big

You could make a copy of the dict then and delete or shorten the big members.

[ThomasWaldmann, 2011-07-07 21:09:28]

http://codereview.appspot.com/4650073/diff/14002/MoinMoin/storage/backends/in...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/14002/MoinMoin/storage/backends/in...
MoinMoin/storage/backends/indexing.py:390: # but other revisions has old name.
why did you remove that comment?

http://codereview.appspot.com/4650073/diff/11003/MoinMoin/search/_tests/test_...
File MoinMoin/search/_tests/test_indexing.py (right):

http://codereview.appspot.com/4650073/diff/11003/MoinMoin/search/_tests/test_...
MoinMoin/search/_tests/test_indexing.py:120: index_object =
WhooshIndex(indexdir=self.index_dir)
be consistent

http://codereview.appspot.com/4650073/diff/3004/MoinMoin/_tests/wikiconfig.py
File MoinMoin/_tests/wikiconfig.py (right):

http://codereview.appspot.com/4650073/diff/3004/MoinMoin/_tests/wikiconfig.py...
MoinMoin/_tests/wikiconfig.py:22: index_dir = os.path.join(data_dir, "index")
i think you'll need a similar thing in the toplevel wikiconfig.py

http://codereview.appspot.com/4650073/diff/3004/MoinMoin/script/maint/index.py
File MoinMoin/script/maint/index.py (right):

http://codereview.appspot.com/4650073/diff/3004/MoinMoin/script/maint/index.p...
MoinMoin/script/maint/index.py:129: Clean given index in index_dir
either always use the short form (ok with me) or always use the long form
app.cfg.index_dir, be consistent.

http://codereview.appspot.com/4650073/diff/3004/MoinMoin/search/_tests/test_i...
File MoinMoin/search/_tests/test_indexing.py (right):

http://codereview.appspot.com/4650073/diff/3004/MoinMoin/search/_tests/test_i...
MoinMoin/search/_tests/test_indexing.py:120: index_object =
WhooshIndex(indexdir=self.index_dir)
be consistent.

http://codereview.appspot.com/4650073/diff/3004/MoinMoin/storage/backends/ind...
File MoinMoin/storage/backends/indexing.py (right):

http://codereview.appspot.com/4650073/diff/3004/MoinMoin/storage/backends/ind...
MoinMoin/storage/backends/indexing.py:391: logging.debug("For add: uuid %s revno
%s" % (metas[UUID], revno))
you mean "To add: ..." maybe?