Christoph Goller wrote:
> Sorry for the delay. I was busy with other things and it took some
> time to fix the problems that Doug mentioned.
No problem. Thanks so much for working on this patch. It looks really
good, and solves a real problem. We can't demand that volunteers work
on a schedule!
My only concerns now are aesthetic. You add segmentInfos as a protected
field of IndexReader, which is a public class. This means it will
appear in the javadoc. Yet this field is of type SegmentInfos, a
package-private class, which will not be in the javadoc.
I try hard to make the javadoc only show things which are meant to be a
part of the public API. So the question is, should SegmentInfos be a
public class, i.e., do we think that folks might need to reference it in
their application code, or should it remain package private?
My rule is that, if you cannot imagine a reasonable use for something,
or if there's an already another public way of doing it, don't make it
public. Keeping more of the implementation private makes it easier to
compatibly evolve the implementation. If they really want, folks can
change things in their private copy of the sources. What we want to
avoid is incompatible changes for folks who only use public APIs. Over
the long term, a smaller public API is easier to maintain.
My preference is that SegmentInfos remain package private. The easiest
way to do this would just be to make the segmentInfos field of
IndexReader package private (i.e. with no public/private declaration).
The only problem is that then IndexReader would not be correctly
subclassable from outside the package.
Alternately, segmentInfos could be made private, will all access from
IndexReader only. Can you see a way to get things to work correctly
this way?
Finally, you add a new constructor to SegmentsReader but the old
constructor is no longer used (except by the new constructor). It would
be better to simply replace the existing constructor with a new
constructor. Less code is better.
If you are tired of working on this, and/or have no more time, please
say so. If you are unable, I will try to address the above issues myself.
Thanks again for all your contributions,
Doug
---------------------------------------------------------------------
To unsubscribe, e-mail: lucene-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: lucene-dev-help@jakarta.apache.org
|