NightOwl888 commented on code in PR #1049:
URL: https://github.com/apache/lucenenet/pull/1049#discussion_r1859108506
##########
src/Lucene.Net/Analysis/TokenAttributes/CharTermAttribute.cs:
##########
@@ -62,35 +61,29 @@ public interface ICharTermAttribute : IAttribute,
ICharSequence, IAppendable
char[] ResizeBuffer(int newSize);
/// <summary>
- /// Gets or Sets the number of valid characters (in
+ /// Gets or sets the number of valid characters (length of the term) in
/// the termBuffer array.
- /// <seealso cref="SetLength(int)"/>
- /// </summary>
- new int Length { get; set; } // LUCENENET: To mimic StringBuilder, we
allow this to be settable.
-
- // LUCENENET specific: Redefining this[] to make it settable
- new char this[int index] { get; set; }
-
- /// <summary>
- /// Set number of valid characters (length of the term) in
- /// the termBuffer array. Use this to truncate the termBuffer
+ /// Use this setter to truncate the termBuffer
/// or to synchronize with external manipulation of the termBuffer.
/// Note: to grow the size of the array,
/// use <see cref="ResizeBuffer(int)"/> first.
- /// NOTE: This is exactly the same operation as calling the <see
cref="Length"/> setter, the primary
- /// difference is that this method returns a reference to the current
object so it can be chained.
- /// <code>
- /// obj.SetLength(30).Append("hey you");
- /// </code>
/// </summary>
- /// <param name="length"> the truncated length </param>
- ICharTermAttribute SetLength(int length);
+ /// <remarks>
+ /// LUCENENET: To mimic StringBuilder, we allow this to be settable.
+ /// This replaces the chainable SetLength method in the Java version.
+ /// </remarks>
+ /// <seealso
cref="Lucene.Net.Analysis.TokenAttributes.Extensions.CharTermAttributeExtensions.SetLength(ICharTermAttribute,
int)"/>
+ new int Length { get; set; }
+
+ // LUCENENET specific: Redefining this[] to make it settable
+ new char this[int index] { get; set; }
/// <summary>
/// Sets the length of the termBuffer to zero.
/// Use this method before appending contents.
/// </summary>
- ICharTermAttribute SetEmpty();
+ /// <seealso
cref="Lucene.Net.Analysis.TokenAttributes.Extensions.CharTermAttributeExtensions.SetEmpty(ICharTermAttribute)"/>
+ void Clear();
Review Comment:
We have already [deviated from upstream on
`IAttribute`](https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.0/lucene/core/src/java/org/apache/lucene/util/Attribute.java)
by [adding a `CopyTo()`
method](https://github.com/apache/lucenenet/blob/bcd49d8c584cdcee480f80fa46c62495740b2bc4/src/Lucene.Net/Util/Attribute.cs#L23).
Like `Clear()`, `CopyTo()` is also an abstract member of `Attribute`. I don't
see any reason why we shouldn't treat `Clear()` the same way.
So, please move this declaration to `IAttribute` instead of here. Since
`ICharTermAttribute` implements `IAttribute`, it will also provide a way to
call `Clear()` from the extension method.
Granted, there may be a good reason not to do this, but I am struggling to
find one. Since an attribute represents a dynamically added piece of data, the
ability to clear that data seems like it will always be a requirement.
Adding `CopyTo()` to `IAttrbute` was a workaround because in .NET we would
require a cast to do it the same way they did in Java, and this seems like a
very similar scenario.
##########
src/Lucene.Net/Support/Analysis/TokenAttributes/Extensions/CharTermAttributeExtensions.cs:
##########
@@ -0,0 +1,73 @@
+/*
Review Comment:
While I think that all of these headers should probably be moved up to the
top of the file like this, currently they have been normalized to be just
inside of the namespace (and indented) in every file of the solution. See:
https://github.com/apache/lucenenet/blob/bcd49d8c584cdcee480f80fa46c62495740b2bc4/src/Lucene.Net/Analysis/TokenAttributes/FlagsAttributeImpl.cs#L7-L22
Please place this license header inside of the namespace like the others for
the time being.
##########
src/Lucene.Net.Tests.Analysis.Common/Analysis/Position/PositionFilterTest.cs:
##########
@@ -105,7 +107,7 @@ public virtual void TestReset()
/// <summary>
/// Tests ShingleFilter up to six shingles against six terms.
/// Tests PositionFilter setting all but the first positionIncrement
to zero. </summary> </exception>
- /// <exception cref="java.io.IOException"> <seealso cref=
Token#next(Token) </seealso>
+ /// <exception cref="IOException"> <seealso cref= Token#next(Token)
</seealso>
Review Comment:
Please also fix the `<seealso>` reference.
##########
src/Lucene.Net/Analysis/Token.cs:
##########
@@ -64,15 +65,15 @@ namespace Lucene.Net.Analysis
/// Failing that, to create a new <see cref="Token"/> you should first use
/// one of the constructors that starts with null text. To load
/// the token from a char[] use <see
cref="ICharTermAttribute.CopyBuffer(char[], int, int)"/>.
- /// To load from a <see cref="string"/> use <see
cref="ICharTermAttribute.SetEmpty()"/> followed by
+ /// To load from a <see cref="string"/> use <see
cref="ICharTermAttribute.SetEmpty()"/> followed by
Review Comment:
Please fix this reference, as it is not resolving.
##########
src/Lucene.Net/Analysis/Token.cs:
##########
@@ -64,15 +65,15 @@ namespace Lucene.Net.Analysis
/// Failing that, to create a new <see cref="Token"/> you should first use
/// one of the constructors that starts with null text. To load
/// the token from a char[] use <see
cref="ICharTermAttribute.CopyBuffer(char[], int, int)"/>.
- /// To load from a <see cref="string"/> use <see
cref="ICharTermAttribute.SetEmpty()"/> followed by
+ /// To load from a <see cref="string"/> use <see
cref="ICharTermAttribute.SetEmpty()"/> followed by
/// <see cref="ICharTermAttribute.Append(string)"/> or <see
cref="ICharTermAttribute.Append(string, int, int)"/>.
/// Alternatively you can get the <see cref="Token"/>'s termBuffer by
calling either <see cref="ICharTermAttribute.Buffer"/>,
/// if you know that your text is shorter than the capacity of the
termBuffer
/// or <see cref="ICharTermAttribute.ResizeBuffer(int)"/>, if there is any
possibility
/// that you may need to grow the buffer. Fill in the characters of your
term into this
/// buffer, with <see cref="string.ToCharArray(int, int)"/> if loading
from a string,
- /// or with <see cref="System.Array.Copy(System.Array, int, System.Array,
int, int)"/>,
- /// and finally call <see cref="ICharTermAttribute.SetLength(int)"/> to
+ /// or with <see cref="System.Array.Copy(System.Array, int, System.Array,
int, int)"/>,
+ /// and finally assign to <see cref="ICharTermAttribute.Length"/> to
Review Comment:
Please change this to read `finally set the <see
cref="ICharTermAttribute.Length"/> of the term text.`
##########
src/Lucene.Net/Analysis/TokenAttributes/CharTermAttributeImpl.cs:
##########
@@ -489,9 +484,7 @@ public override void CopyTo(IAttribute target) // LUCENENET
specific - intention
char[] ICharTermAttribute.ResizeBuffer(int newSize) =>
ResizeBuffer(newSize);
- ICharTermAttribute ICharTermAttribute.SetLength(int length) =>
SetLength(length);
-
- ICharTermAttribute ICharTermAttribute.SetEmpty() => SetEmpty();
+ void ICharTermAttribute.Clear() => Clear();
Review Comment:
We technically only need these explicit declarations to change the return
type of the public methods that implement the interface. Since this returns
`void`, we don't need it.
--
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]
