[Xapian-discuss] External Source Post List

Olly Betts olly at survex.com
Wed Feb 21 20:18:35 GMT 2007


On Wed, Feb 21, 2007 at 12:06:00PM -0700, Rusty Conover wrote:
> Here are links to the patches:
> 
> http://www.infogears.com/patches/externalsourcepostlist.patch

OK, I found my comments on the previous version you sent:

http://article.gmane.org/gmane.comp.search.xapian.devel/742

I think you've since addressed all of those except:

* ExternalPostingSource should derive from RefCntBase and then you can
  use the same reference counting mechanism the other API classes do.

So instead of:

    Xapian::ExternalPostingSource *external_source;

you'd have:

    Xapian::RefCntPtr<Xapian::ExternalPostingSource> external_source;

Then all the reference counting happens as if by magic!

There are a few places that could do cleaning up a little - one hunk
just adds several blank lines to a function, and another adds a trailing
comma to an enum.

The ExternalPostingSource class and its methods ought to have
documentation comments so they are documented in the API documentation
which doxygen generates.  These look like:

/** Brief description.
 *
 *  Longer description.
 */

There's various markup you can use - see the existing documentation
comments for examples, or see the doxygen manual.

It would also be good to have some simple feature tests for the new
feature.  A good test to copy is probably "matchfunctor1" in
"tests/api_db.cc".

This also won't apply cleanly to SVN HEAD, if only because I've reworked
the build system to be mostly non-recursive.

I can probably cope with doing some of the tweaking, though it might be
a while before I get to it.  The documentation comments and test cases
are probably the most useful thing for you to provide, as they're the
hardest for me to fill in.

> http://www.infogears.com/patches/externalsourcepostlist-perl.patch

I've not looked at this in much detail, but adding the ability to pass
"check_at_least" is fine.  I think it's only not wrapped because the
Search::Xapian wrappers pre-date the addition of check_at_least.

It's great that you seem to have wrapped MatchDecider such that it
can be subclassed in Perl (unless I misunderstood the patch).  Several
people have asked about that in the past.

I'll see if I can merge the non-externalsource parts of the patch soon
and make a new Search::Xapian point release (the externalsource parts
need to wait for xapian-core).

Cheers,
    Olly



More information about the Xapian-discuss mailing list