Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PersianStemmer #571

Merged
merged 5 commits into from
May 22, 2022
Merged

Add PersianStemmer #571

merged 5 commits into from
May 22, 2022

Conversation

raminmjj
Copy link
Contributor

No description provided.

@NightOwl888
Copy link
Contributor

Thanks for the PR.

You have done a great job following conventions and putting this together. However, PersianStemmer is not something that already exists in Lucene and we don't have an approval process set up for this sort of thing in .NET (yet).

Options

  1. If you would like to see this released in Lucene.NET sooner rather than later, you can port this great contribution to Java and submit it to the Lucene repository on GitHub. See their Contributing guide for more info.
  2. Alternatively, we can port this to Java and submit the contribution for you (when we get a chance to) and will include it in a future Lucene.NET 4.8.0 release, even though it didn't exist in Java at this version.

Either way, it would be great to keep this PR open and to report the status of the contribution to Lucene back here so we can track the progress and learn about how these sort of approvals work. We will merge it once the Lucene team makes the final approval.

@raminmjj
Copy link
Contributor Author

Hi Shad,
I try to port this to Java.
Thank you.

@NightOwl888
Copy link
Contributor

Great. Let us know if you need help with that. The Reader has been moved to a different place in Lucene analyzers, but other than that, I think it will be straightforward to do.

@raminmjj
Copy link
Contributor Author

raminmjj commented Mar 2, 2022

I made a pull request 3 months ago. And I'm waiting for them :)

Copy link
Contributor

@NightOwl888 NightOwl888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this. There are just a few minor changes to do before this can be merged.

@raminmjj raminmjj requested a review from NightOwl888 May 12, 2022 12:59
@raminmjj
Copy link
Contributor Author

Hi Shad.
I implemented your suggestions.
Please review the changes.

Copy link
Contributor

@NightOwl888 NightOwl888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. There is one more requested change that has not been resolved, calling Arrays.Equals() to do the char[] comparison.

@NightOwl888
Copy link
Contributor

@raminmjj - Looks like the Lucene team decided not to make the PersianStemmer part of the built-in PersianAnalyzer, but expect users to custom build an analyzer if they want to utilize it: apache/lucene#904

I will make the same changes to Lucene.NET in a new PR.

@NightOwl888 NightOwl888 merged commit c7ab459 into apache:master May 22, 2022
NightOwl888 added a commit to NightOwl888/lucenenet that referenced this pull request May 22, 2022
 as was done in apache/lucene#904. Changed TestPersianStemFilter to use mocks.
@raminmjj raminmjj deleted the PersianStemmer branch May 27, 2022 06:05
@mocobeta
Copy link

Hi,

Looks like the Lucene team decided not to make the PersianStemmer part of the built-in PersianAnalyzer, but expect users to custom build an analyzer if they want to utilize it: apache/lucene#904

We removed PersianStemmer from PersianAnalyzer only for 9x branch (current maintenance branch) to avoid breaking the backward compatibility in analysis in 9.x, but the change was not reverted from main and will be released in the next major release.

@raminmjj raminmjj restored the PersianStemmer branch June 1, 2022 08:05
NightOwl888 added a commit that referenced this pull request Oct 20, 2022
… was done in apache/lucene#904. Changed TestPersianStemFilter to use mocks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants