NightOwl888 commented on code in PR #1049:
URL: https://github.com/apache/lucenenet/pull/1049#discussion_r1857951550
##########
src/Lucene.Net.TestFramework/Index/BaseTermVectorsFormatTestCase.cs:
##########
@@ -360,7 +360,8 @@ public sealed override bool IncrementToken()
{
if (i < terms.Length)
{
- termAtt.SetLength(0).Append(terms[i]);
+ termAtt.Length = 0;
Review Comment:
The idea is to add an extension method to fill the gap so we can still use a
fluent API like above, but remove the method as a required member to implement
with `ICharTermAttribute`. Please add the `SetLength()` extension method in a
new class at
`Lucene.Net/Support/Analysis/TokenAttributes/Extensions/CharTermAttributeExtensions.cs`
. Use the namespace `Lucene.Net.Analysis.TokenAttributes.Extensions`. This
will make its usage optional by importing the namespace. Use the
`ICharTermAttribute` interface rather than `CharTermAttribute` as the `this`
(target) type for the extension method. Move the guard clauses into the setter
of `Length` so it doesn't have to be duplicated in the extension method.
------
We will be migrating other methods into this namespace, such as the
`SetOffset()` method from `IOffsetAttribute` once we add a settable `Length`
property that can be set independent of `StartOffset`. `EndOffset` can be kept
readonly and then there is no real need for `SetOffset()` except for Java
Lucene compatibility, so it can be an optional extension method like this one.
Note that we took a similar approach to this on the `IndexWriterConfig`
class, changing all of the [setters and getters to
properties](https://github.com/apache/lucenenet/blob/ccff6ddf6ab37cc699ef978abaff4d28f7a8ce23/src/Lucene.Net/Index/IndexWriterConfig.cs#L279-L282)
and migrating the [fluent
API](https://github.com/apache/lucenenet/blob/ccff6ddf6ab37cc699ef978abaff4d28f7a8ce23/src/Lucene.Net/Support/Index/Extensions/IndexWriterConfigExtensions.cs#L295-L299)
to extension methods that are optional to import. So this makes the project
more consistent. All of the tests still use the fluent API to stay aligned with
the upstream tests (note this tests the properties simultaneously).
> **NOTE:** We are missing the `null` guard clauses in the extension
methods, but we should throw `ArgumentNullException` rather than relying on
`NullReferenceException`. `ArgumentNullException` is a design choice, but
`NullReferenceException` is a bug. This can be made into a separate issue for
`IndexWriterConfigExtensions`, but please add a guard clause in the extension
method for this PR.
The `ICharTermAttribute.SetEmpty()` method could probably also be eliminated
from the interface, replaced with a `Clear()` method, and made into an
extension method. This would make `ICharTermAttribute` easier to understand
when not used as a fluent API because it matches `StringBuilder` and collection
types. The downside is that it would add an additional method that does the
same operation. The upside is it would neatly make all of the fluent APIs into
extension methods while keeping the interface and implementation free from
fluent APIs.
--
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]
