[GitHub] [lucenenet] NightOwl888 commented on pull request #654: fix: Aligned FrenchStemmer.cs with Java code

[GitBox <gi...@apache.org>]

NightOwl888 commented on PR #654:
URL: https://github.com/apache/lucenenet/pull/654#issuecomment-1279077333

   > > Well, you did find an issue, just not the one you thought 😁. It might be cleaner to close #654 and #655 and open a new PR (or PRs as noted below) fixing the order of precedence issue throughout the solution as well as switching to the extension methods in J2N noted below.
   > > Since all of the stemmers (and other files in `Lucene.Net.Analysis.Common`) were ported at the same time using an automated Java to C# converter, it wouldn't surprise me if this same precedence issue exists across all of the `.Remove(startIndex, length)` calls in the entire project.
   > > Also since the time `Lucene.Net.Analysis.Common` was ported, we have added extension methods for [`Replace()`](https://github.com/NightOwl888/J2N/blob/v2.0.0/src/J2N/Text/StringBuilderExtensions.cs#L1042) and [`Delete()`](https://github.com/NightOwl888/J2N/blob/v2.0.0/src/J2N/Text/StringBuilderExtensions.cs#L397) in J2N that take into account very subtle differences in behavior. Be aware that they use .NET semantics `startIndex` and `length`, since it would be unwieldly to convert between 2 conventions all the time in the same application.
   > > A type of review you could do is to track down as many of the methods as you can that have to make this conversion from start/end to startIndex/length, check their order of precedence conversion, and change their usage to use the methods in J2N, if appropriate. The best place to start is to compare all of the `String` and `StringBuilder` method parameters between .NET and Java, since they are the most critical for Lucene.NET. If you prefer to submit 1 PR per method review across the entire project, that is fine.
   > > And if it is sensible to add a new extension method, you may submit a PR to J2N with full documentation (generally copied from .NET) and tests ported from JavaSE 8. Note that the API should match similar methods in .NET, but the behavior should match JavaSE 8.
   > 
   > I've now looked for other occurrences of this error but was unable to find any. Therefore, I think the replacement of the methods with the ones found in J2N might be a better candidate for creating an issue with a link to this PR and then we can open PR's targeting the replacement specifically 😄
   > 
   > I've also pushed a commit with the parentheses fixes to this PR.
   > 
   > And just to be sure then the idea would be to replace:
   > 
   > * `StringBuilder.Remove` replaced with `StringBuilder.Delete` (From J2N)
   > * `StringBuilder.Remove` followed by `StringBuilder.Insert` replaced with `StringBuilder.Replace` (From J2N)
   
   Yes, those conversions are correct.


-- 
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: dev-unsubscribe@lucenenet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org