You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/05/11 23:36:09 UTC
[GitHub] [lucene] shahrs87 opened a new pull request, #883: LUCENE-10561 Reduce class/member visibility of all normalizer and stemmer classes
shahrs87 opened a new pull request, #883:
URL: https://github.com/apache/lucene/pull/883
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Lucene:
* https://issues.apache.org/jira/projects/LUCENE
You will need to create an account in Jira in order to create an issue.
The title of the PR should reference the Jira issue number in the form:
* LUCENE-####: <short description of problem or changes>
LUCENE must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
Please provide a short description of the changes you're making with this pull request.
# Solution
Please provide a short description of the approach taken to implement your solution.
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/lucene/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [ ] I have given Lucene maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mocobeta commented on pull request #883: LUCENE-10561 Reduce class/member visibility of all normalizer and stemmer classes
Posted by GitBox <gi...@apache.org>.
mocobeta commented on PR #883:
URL: https://github.com/apache/lucene/pull/883#issuecomment-1125910079
@shahrs87 Looks good to me. Can you please add a [CHANGES](https://github.com/apache/lucene/blob/main/lucene/CHANGES.txt) entry to "API changes" section in 10.0.0? Also, we need a [MIGRATE](https://github.com/apache/lucene/blob/main/lucene/MIGRATE.md) entry; I'll add it later.
I think we can merge this with CHANGES and MIGRATE entries. Will keep it open till tomorrow to let others give time for another review.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] shahrs87 commented on pull request #883: LUCENE-10561 Reduce class/member visibility of all normalizer and stemmer classes
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on PR #883:
URL: https://github.com/apache/lucene/pull/883#issuecomment-1126220894
> Can you please add a [CHANGES](https://github.com/apache/lucene/blob/main/lucene/CHANGES.txt) entry to "API changes" section in 10.0.0?
@mocobeta Done ! Thank you for the review.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mocobeta commented on pull request #883: LUCENE-10561 Reduce class/member visibility of all normalizer and stemmer classes
Posted by GitBox <gi...@apache.org>.
mocobeta commented on PR #883:
URL: https://github.com/apache/lucene/pull/883#issuecomment-1125583595
I feel it's a bit confusing that package-private classes have `public` members. Should we also change all public variables/methods to package-private (in other words, remove the `public` access modifier from those classes)?
cc @rmuir
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mocobeta commented on pull request #883: LUCENE-10561 Reduce class/member visibility of all normalizer and stemmer classes
Posted by GitBox <gi...@apache.org>.
mocobeta commented on PR #883:
URL: https://github.com/apache/lucene/pull/883#issuecomment-1125578473
> Couldn't change for StempelStemmer since PolishAnalyzer depends on some member variable of StempelStemmer
`StempelStemmer` is supposed to be used from other packages/modules (according to the javadocs), I think it's fine.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] shahrs87 commented on pull request #883: LUCENE-10561 Reduce class/member visibility of all normalizer and stemmer classes
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on PR #883:
URL: https://github.com/apache/lucene/pull/883#issuecomment-1125684436
@mocobeta Can you please suggest some starter jiras which will give me overview on Lucene and will be helpful to community also. Thank you.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] shahrs87 commented on pull request #883: LUCENE-10561 Reduce class/member visibility of all normalizer and stemmer classes
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on PR #883:
URL: https://github.com/apache/lucene/pull/883#issuecomment-1125674862
@mocobeta Thank you for your review. Addressed your review comment in latest revision. Please review again.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] shahrs87 commented on pull request #883: LUCENE-10561 Reduce class/member visibility of all normalizer and stemmer classes
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on PR #883:
URL: https://github.com/apache/lucene/pull/883#issuecomment-1125209240
Hi @mocobeta
I have updated the PR as per your's and @rmuir's feedback. I have reduced class visibility of most of normalizer and stemmer related classes. Couldn't change for `StempelStemmer` since `PolishAnalyzer` depends on some member variable of `StempelStemmer`
Please review again.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mocobeta commented on pull request #883: LUCENE-10561 Reduce class/member visibility of all normalizer and stemmer classes
Posted by GitBox <gi...@apache.org>.
mocobeta commented on PR #883:
URL: https://github.com/apache/lucene/pull/883#issuecomment-1124499151
Hi @shahrs87.
First of all, thanks for the great PR! As for the failed checksum check for `ClassicTokenizerImpl`, please refer to Robert's comment in Jira - I think you can omit `ClassicTokenizerImpl` for now and we can improve it in another issue/PR.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] shahrs87 commented on pull request #883: LUCENE-10561 Reduce class/member visibility of all normalizer and stemmer classes
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on PR #883:
URL: https://github.com/apache/lucene/pull/883#issuecomment-1124385828
```
apache-lucene % ./gradlew check
> Task :lucene:analysis:common:generateClassicTokenizerChecksumCheck FAILED
FAILURE: Build failed with an exception.
* Where:
Script '/Users/rushabh.shah/apache-lucene/gradle/generation/regenerate.gradle' line: 184
* What went wrong:
Execution failed for task ':lucene:analysis:common:generateClassicTokenizerChecksumCheck'.
> Checksums mismatch for derived resources; you might have modified a generated resource (regenerate task: generateClassicTokenizer):
Current:
lucene/analysis/common/src/java/org/apache/lucene/analysis/classic/ClassicTokenizerImpl.java=c825a8b8d0d0d893b4914e7161bcd119e7b07b40
Expected:
lucene/analysis/common/src/java/org/apache/lucene/analysis/classic/ClassicTokenizerImpl.java=381a9627fd7da6402216e3279cf81a09af222aaf
```
When I am running `./gradlew check`, I am getting the above error. Looks like I need to update checksum in some file. Do we have any graddle target to achieve that ?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] shahrs87 commented on pull request #883: LUCENE-10561 Reduce class/member visibility of all normalizer and stemmer classes
Posted by GitBox <gi...@apache.org>.
shahrs87 commented on PR #883:
URL: https://github.com/apache/lucene/pull/883#issuecomment-1124521767
@mocobeta Thank you the reply. I have removed the classes that are generated via jflex. I also ran all the workflow test mentioned in Contributors guide and all seems to pass. Can you please review the PR ? Thank you.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mocobeta merged pull request #883: LUCENE-10561 Reduce class/member visibility of all normalizer and stemmer classes
Posted by GitBox <gi...@apache.org>.
mocobeta merged PR #883:
URL: https://github.com/apache/lucene/pull/883
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mocobeta commented on pull request #883: LUCENE-10561 Reduce class/member visibility of all normalizer and stemmer classes
Posted by GitBox <gi...@apache.org>.
mocobeta commented on PR #883:
URL: https://github.com/apache/lucene/pull/883#issuecomment-1125928019
> Can you please suggest some starter jiras which will give me overview on Lucene and will be helpful to community also.
We have a couple of Jira issues with `newdev` label for new developers but I don't think the list is well maintained, sorry :/
https://issues.apache.org/jira/browse/LUCENE-9303?jql=(project%3DLUCENE)%20AND%20resolution%3DUnresolved%20AND%20labels%3Dnewdev
Apart from `newdev` label, there are many small (minor) issues that are non-controversial, you could browse unresolved and well-described Jira issues and then pick ones for starters.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org