NightOwl888 commented on code in PR #819:
URL: https://github.com/apache/lucenenet/pull/819#discussion_r1163923121
##########
src/Lucene.Net/Codecs/BlockTreeTermsReader.cs:
##########
@@ -82,11 +82,11 @@ namespace Lucene.Net.Codecs
/// option to see summary statistics on the blocks in the
/// dictionary.</para>
///
- /// See <see cref="BlockTreeTermsWriter"/>.
+ /// See <see cref="BlockTreeTermsWriter{T}"/>.
Review Comment:
Please update this XML doc comment to match `TSubclassState`
##########
src/Lucene.Net/Codecs/BlockTreeTermsReader.cs:
##########
@@ -114,15 +114,36 @@ public class BlockTreeTermsReader : FieldsProducer
private readonly int version;
+ protected readonly TSubclassState m_subclassState;
+
/// <summary>
/// Sole constructor. </summary>
- public BlockTreeTermsReader(Directory dir, FieldInfos fieldInfos,
SegmentInfo info, PostingsReaderBase postingsReader, IOContext ioContext,
string segmentSuffix, int indexDivisor)
+ /// <param name="subclassState">LUCENENET specific parameter which
allows a subclass
+ /// to set state. It is *optional* and can be used when overriding the
ReadHeader(),
+ /// ReadIndexHeader() and SeekDir() methods. It only matters in the
case where the state
+ /// is required inside of any of those methods that is passed in to
the subclass constructor.
+ ///
+ /// When passed to the constructor, it is set to the protected field
m_subclassState before
+ /// any of the above methods are called where it is available for
reading when overriding the above methods.
+ ///
+ /// If your subclass needs to pass more than one piece of data, you
can create a class or struct to do so.
+ /// All other virtual members of BlockTreeTermsReader are not called
in the constructor,
+ /// so the overrides of those methods won't specifically need to use
this field (although they could for consistency).
+ /// </param>
+ [SuppressMessage("CodeQuality", "IDE0079:Remove unnecessary
suppression", Justification = "This is a SonarCloud issue")]
+ [SuppressMessage("CodeQuality", "S1699:Constructors should only call
non-overridable methods", Justification = "Internal class")]
Review Comment:
Let's change the justification here to "Required for continuity with
Lucene's design".
##########
src/Lucene.Net.Codecs/Pulsing/Pulsing41PostingsFormat.cs:
##########
@@ -35,7 +35,7 @@ public Pulsing41PostingsFormat()
/// <summary>Inlines docFreq=<paramref name="freqCutoff"/> terms,
otherwise uses the normal "Lucene41" format.</summary>
public Pulsing41PostingsFormat(int freqCutoff)
- : this(freqCutoff, BlockTreeTermsWriter.DEFAULT_MIN_BLOCK_SIZE,
BlockTreeTermsWriter.DEFAULT_MAX_BLOCK_SIZE)
+ : this(freqCutoff,
BlockTreeTermsWriter<object>.DEFAULT_MIN_BLOCK_SIZE,
BlockTreeTermsWriter<object>.DEFAULT_MAX_BLOCK_SIZE)
Review Comment:
Usually when I run into ambiguity like this due to generics, I move the
constant into a non-generic static class with the same name so it can still be
accessed via `BlockTreeTermsWriter.DEFAULT_MIN_BLOCK_SIZE`. It isn't as nice on
the inside of the class, but everybody else sees it in the same place. IMO, it
makes more sense than having an `<object>` argument that is completely
meaningless, but it isn't 100% clear that it is meaningless when specifying
these constants in the code.
```c#
public static class BlockTreeTermsWriter
{
/// <summary>
/// Suggested default value for the
/// <c>minItemsInBlock</c> parameter to
/// <see cref="BlockTreeTermsWriter(SegmentWriteState,
PostingsWriterBase, int, int)"/>.
/// </summary>
public const int DEFAULT_MIN_BLOCK_SIZE = 25;
/// <summary>
/// Suggested default value for the
/// <c>maxItemsInBlock</c> parameter to
/// <see cref="BlockTreeTermsWriter(SegmentWriteState,
PostingsWriterBase, int, int)"/>.
/// </summary>
public const int DEFAULT_MAX_BLOCK_SIZE = 48;
}
```
There may be other constants that would be useful to move to this static
class to reduce the number of these ambiguous calls.
We should also move the 2 public nested classes
`BlockTreeTermsReader.FieldReader` and `BlockTreeTermsReader.Stats` so they can
be found in the same place on the public API. Be sure to drop a comment in the
place where they were moved from so we understand when looking at the Java code
file where they have gone.
```c#
// LUCENENET specific - moved Stats class inside static BlockTreeTermsReader
class so it can be used without a generic closing type.
```
##########
src/Lucene.Net/Codecs/BlockTreeTermsWriter.cs:
##########
@@ -178,20 +179,20 @@ namespace Lucene.Net.Codecs
/// <para/>
/// @lucene.experimental
/// </summary>
- /// <seealso cref="BlockTreeTermsReader"/>
- public class BlockTreeTermsWriter : FieldsConsumer
+ /// <seealso cref="BlockTreeTermsReader{T}"/>
+ public class BlockTreeTermsWriter<TSubclassState> : FieldsConsumer
{
/// <summary>
/// Suggested default value for the
/// <c>minItemsInBlock</c> parameter to
- /// <see cref="BlockTreeTermsWriter(SegmentWriteState,
PostingsWriterBase, int, int)"/>.
+ /// <see cref="BlockTreeTermsWriter{T}(SegmentWriteState,
PostingsWriterBase, int, int, T)"/>.
Review Comment:
Please update this XML doc comment to match `TSubclassState` in both places
##########
src/Lucene.Net/Codecs/BlockTreeTermsWriter.cs:
##########
@@ -178,20 +179,20 @@ namespace Lucene.Net.Codecs
/// <para/>
/// @lucene.experimental
/// </summary>
- /// <seealso cref="BlockTreeTermsReader"/>
- public class BlockTreeTermsWriter : FieldsConsumer
+ /// <seealso cref="BlockTreeTermsReader{T}"/>
+ public class BlockTreeTermsWriter<TSubclassState> : FieldsConsumer
{
/// <summary>
/// Suggested default value for the
/// <c>minItemsInBlock</c> parameter to
- /// <see cref="BlockTreeTermsWriter(SegmentWriteState,
PostingsWriterBase, int, int)"/>.
+ /// <see cref="BlockTreeTermsWriter{T}(SegmentWriteState,
PostingsWriterBase, int, int, T)"/>.
/// </summary>
public const int DEFAULT_MIN_BLOCK_SIZE = 25;
/// <summary>
/// Suggested default value for the
/// <c>maxItemsInBlock</c> parameter to
- /// <see cref="BlockTreeTermsWriter(SegmentWriteState,
PostingsWriterBase, int, int)"/>.
+ /// <see cref="BlockTreeTermsWriter{T}(SegmentWriteState,
PostingsWriterBase, int, int, T)"/>.
Review Comment:
Please update this XML doc comment to match `TSubclassState` in both places
##########
src/Lucene.Net/Codecs/BlockTreeTermsWriter.cs:
##########
@@ -178,20 +179,20 @@ namespace Lucene.Net.Codecs
/// <para/>
/// @lucene.experimental
/// </summary>
- /// <seealso cref="BlockTreeTermsReader"/>
- public class BlockTreeTermsWriter : FieldsConsumer
+ /// <seealso cref="BlockTreeTermsReader{T}"/>
Review Comment:
Please update this XML doc comment to match `TSubclassState`
##########
src/Lucene.Net/Codecs/Lucene40/Lucene40PostingsFormat.cs:
##########
@@ -87,7 +87,7 @@ namespace Lucene.Net.Codecs.Lucene40
/// <a name="Termindex" id="Termindex"></a>
/// <h3>Term Index</h3>
/// <para>The .tip file contains an index into the term dictionary, so
that it can be
- /// accessed randomly. See <see cref="BlockTreeTermsWriter"/> for more
details on the format.</para>
+ /// accessed randomly. See <see cref="BlockTreeTermsWriter{T}"/> for more
details on the format.</para>
Review Comment:
Please update this XML doc comment to match `TSubclassState`
##########
src/Lucene.Net/Codecs/PostingsWriterBase.cs:
##########
@@ -27,7 +27,7 @@ namespace Lucene.Net.Codecs
/// Extension of <see cref="PostingsConsumer"/> to support pluggable term
dictionaries.
/// <para/>
/// This class contains additional hooks to interact with the provided
- /// term dictionaries such as <see cref="BlockTreeTermsWriter"/>. If you
want
+ /// term dictionaries such as <see cref="BlockTreeTermsWriter{T}"/>. If
you want
Review Comment:
Please update this XML doc comment to match `TSubclassState`
##########
src/Lucene.Net/Codecs/Lucene41/Lucene41PostingsFormat.cs:
##########
@@ -104,7 +104,7 @@ namespace Lucene.Net.Codecs.Lucene41
/// field along with per-term statistics (such as docfreq)
/// and pointers to the frequencies, positions, payload and
/// skip data in the .doc, .pos, and .pay files.
- /// See <see cref="BlockTreeTermsWriter"/> for more details on the format.
+ /// See <see cref="BlockTreeTermsWriter{T}"/> for more details on the
format.
Review Comment:
Please update this XML doc comment to match `TSubclassState`
##########
src/Lucene.Net/Codecs/Lucene41/Lucene41PostingsFormat.cs:
##########
@@ -161,7 +161,7 @@ namespace Lucene.Net.Codecs.Lucene41
/// <dd>
/// <b>Term Index</b>
/// <para>The .tip file contains an index into the term dictionary, so
that it can be
- /// accessed randomly. See <see cref="BlockTreeTermsWriter"/> for more
details on the format.</para>
+ /// accessed randomly. See <see cref="BlockTreeTermsWriter{T}"/> for more
details on the format.</para>
Review Comment:
Please update this XML doc comment to match `TSubclassState`
##########
src/Lucene.Net/Codecs/Lucene40/Lucene40PostingsFormat.cs:
##########
@@ -221,15 +221,15 @@ public class Lucene40PostingsFormat : PostingsFormat
/// settings.
/// </summary>
public Lucene40PostingsFormat()
- : this(BlockTreeTermsWriter.DEFAULT_MIN_BLOCK_SIZE,
BlockTreeTermsWriter.DEFAULT_MAX_BLOCK_SIZE)
+ : this(BlockTreeTermsWriter<object>.DEFAULT_MIN_BLOCK_SIZE,
BlockTreeTermsWriter<object>.DEFAULT_MAX_BLOCK_SIZE)
{
}
/// <summary>
/// Creates <see cref="Lucene40PostingsFormat"/> with custom
/// values for <paramref name="minBlockSize"/> and
/// <paramref name="maxBlockSize"/> passed to block terms dictionary.
</summary>
- /// <seealso
cref="BlockTreeTermsWriter.BlockTreeTermsWriter(SegmentWriteState,PostingsWriterBase,int,int)"/>
+ /// <seealso
cref="BlockTreeTermsWriter{T}.BlockTreeTermsWriter(SegmentWriteState,PostingsWriterBase,int,int,T)"/>
Review Comment:
Please update this XML doc comment to match `TSubclassState` in both places
##########
src/Lucene.Net/Codecs/Lucene40/Lucene40PostingsFormat.cs:
##########
@@ -42,7 +42,7 @@ namespace Lucene.Net.Codecs.Lucene40
/// field along with per-term statistics (such as docfreq)
/// and pointers to the frequencies, positions and
/// skip data in the .frq and .prx files.
- /// See <see cref="BlockTreeTermsWriter"/> for more details on the format.
+ /// See <see cref="BlockTreeTermsWriter{T}"/> for more details on the
format.
Review Comment:
Please update this XML doc comment to match `TSubclassState`
##########
src/Lucene.Net/Codecs/BlockTreeTermsWriter.cs:
##########
@@ -279,14 +280,33 @@ public FieldMetaData(FieldInfo fieldInfo, BytesRef
rootCode, long numTerms, long
private readonly IList<FieldMetaData> fields = new
JCG.List<FieldMetaData>();
// private final String segment;
+ protected object m_subclassState = null;
+
/// <summary>
/// Create a new writer. The number of items (terms or
/// sub-blocks) per block will aim to be between
- /// <paramref name="minItemsInBlock"/> and <paramref
name="maxItemsInBlock"/>, though in some
- /// cases the blocks may be smaller than the min.
/// </summary>
- public BlockTreeTermsWriter(SegmentWriteState state,
PostingsWriterBase postingsWriter, int minItemsInBlock, int maxItemsInBlock)
+ /// <param name="subclassState">LUCENENET specific parameter which
allows a subclass
+ /// to set state. It is *optional* and can be used when overriding the
WriteHeader(),
+ /// WriteIndexHeader(). It only matters in the case where the state
+ /// is required inside of any of those methods that is passed in to
the subclass constructor.
+ ///
+ /// When passed to the constructor, it is set to the protected field
m_subclassState before
+ /// any of the above methods are called where it is available for
reading when overriding the above methods.
+ ///
+ /// If your subclass needs to pass more than one piece of data, you
can create a class or struct to do so.
+ /// All other virtual members of BlockTreeTermsWriter are not called
in the constructor,
+ /// so the overrides of those methods won't specifically need to use
this field (although they could for consistency).
+ /// </param>
+ [SuppressMessage("CodeQuality", "IDE0079:Remove unnecessary
suppression", Justification = "This is a SonarCloud issue")]
+ [SuppressMessage("CodeQuality", "S1699:Constructors should only call
non-overridable methods", Justification = "Internal class")]
Review Comment:
Let's change the justification here to "Required for continuity with
Lucene's design".
##########
src/Lucene.Net/Codecs/Lucene41/Lucene41PostingsFormat.cs:
##########
@@ -379,15 +379,15 @@ public sealed class Lucene41PostingsFormat :
PostingsFormat
/// settings.
/// </summary>
public Lucene41PostingsFormat()
- : this(BlockTreeTermsWriter.DEFAULT_MIN_BLOCK_SIZE,
BlockTreeTermsWriter.DEFAULT_MAX_BLOCK_SIZE)
+ : this(BlockTreeTermsWriter<object>.DEFAULT_MIN_BLOCK_SIZE,
BlockTreeTermsWriter<object>.DEFAULT_MAX_BLOCK_SIZE)
{
}
/// <summary>
/// Creates <see cref="Lucene41PostingsFormat"/> with custom
/// values for <paramref name="minTermBlockSize"/> and
/// <paramref name="maxTermBlockSize"/> passed to block terms
dictionary. </summary>
- /// <seealso
cref="BlockTreeTermsWriter.BlockTreeTermsWriter(SegmentWriteState,PostingsWriterBase,int,int)"/>
+ /// <seealso
cref="BlockTreeTermsWriter{T}.BlockTreeTermsWriter(SegmentWriteState,PostingsWriterBase,int,int,T)"/>
Review Comment:
Please update this XML doc comment to match `TSubclassState` in both places
##########
src/Lucene.Net/Codecs/PostingsReaderBase.cs:
##########
@@ -29,7 +29,7 @@ namespace Lucene.Net.Codecs
/// <summary>
/// The core terms dictionaries (BlockTermsReader,
- /// <see cref="BlockTreeTermsReader"/>) interact with a single instance
+ /// <see cref="BlockTreeTermsReader{T}"/>) interact with a single instance
Review Comment:
Please update this XML doc comment to match `TSubclassState`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
