[GitHub] [lucene] apanimesh061 commented on pull request #412: LUCENE-10197: UnifiedHighlighter should use builders for thread-safety
apanimesh061 commented on pull request #412: URL: https://github.com/apache/lucene/pull/412#issuecomment-997424641 @dsmiley I've made the suggested modifications. -- 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-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] apanimesh061 commented on pull request #412: LUCENE-10197: UnifiedHighlighter should use builders for thread-safety
apanimesh061 commented on pull request #412: URL: https://github.com/apache/lucene/pull/412#issuecomment-997280739 @dsmiley I've made an attempt at making this back-portable as suggested. - `@Deprecated` the setters and constructor - Have not added any tests for the non-builder case - Existing tests are working -- 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-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] apanimesh061 commented on pull request #412: LUCENE-10197: UnifiedHighlighter should use builders for thread-safety
apanimesh061 commented on pull request #412: URL: https://github.com/apache/lucene/pull/412#issuecomment-988169852 @dsmiley In order to make this portable to v9.x should we make 2 unit test suites for "with builder" and "w/o builder"? It will just be easier to remove the non-builder files and rename the builder based tests in v10.x. -- 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-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] apanimesh061 commented on pull request #412: LUCENE-10197: UnifiedHighlighter should use builders for thread-safety
apanimesh061 commented on pull request #412: URL: https://github.com/apache/lucene/pull/412#issuecomment-986360914 > I think this is done! > > Please add an entry to lucene/CHANGES.txt. I propose the following, under "API Changes": > > ``` > * LUCENE-10197: UnifiedHighlighter now has a mandatory Builder to construct it. The UH is now immutable. (your-name, David Smiley) > ``` @dsmiley That is great. I've added the entry to the file under `API Changes` for `Lucene 10.0.0`. -- 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-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] apanimesh061 commented on pull request #412: LUCENE-10197: UnifiedHighlighter should use builders for thread-safety
apanimesh061 commented on pull request #412: URL: https://github.com/apache/lucene/pull/412#issuecomment-986174116 @dsmiley Updated the PR as per the comments and added the new builder functions. -- 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-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] apanimesh061 commented on pull request #412: LUCENE-10197: UnifiedHighlighter should use builders for thread-safety
apanimesh061 commented on pull request #412: URL: https://github.com/apache/lucene/pull/412#issuecomment-981097369 @dsmiley I was finally able to address your comments on my previous PR request. Thank you for your patience. Added a new PR version with new changes related to the builder. Please take a look whenever you get a chance. Following commands work: ``` ./gradlew preCommit ./gradlew check ``` New version includes: - Non-abstract builder with no "concreteBuilder" - More java-doc statements in the UH class - Modified unit-tests -- 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-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] apanimesh061 commented on pull request #412: LUCENE-10197: UnifiedHighlighter should use builders for thread-safety
apanimesh061 commented on pull request #412:
URL: https://github.com/apache/lucene/pull/412#issuecomment-955779857
PR updated
Included:
- Removal of setters and their references from any unit test or class
- Replace UH setters/constructors with the new builder
- Added test UH extensibility
- `flags` are evaluated in the builder
Not included:
- `getFieldMatcher(String field)` is not a getter right now
- No changes related to Javadocs
--
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-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] apanimesh061 commented on pull request #412: LUCENE-10197: UnifiedHighlighter should use builders for thread-safety
apanimesh061 commented on pull request #412: URL: https://github.com/apache/lucene/pull/412#issuecomment-955147804 The new changes include the following: 1. if `flags` is initialized then boolean flags are ignored 2. Unit test for new `getFlags()` implementation using builders 3. Setter for `flags` 4. Builder can used to set booleans as well as `flags` Have not implemented the randomized testing as suggested. Still researching into how to implement 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-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] apanimesh061 commented on pull request #412: LUCENE-10197: UnifiedHighlighter should use builders for thread-safety
apanimesh061 commented on pull request #412: URL: https://github.com/apache/lucene/pull/412#issuecomment-95323 > It looks good at a glance... you/I will see better if you update one of the clients that might want to subclass with extra configuration. Is there any or is this builder subclassing issue entirely hypothetical at this point? I suspect only hypothetical. We'll want nice Javadocs on the builder setters since this is where consumers/clients will see it. We can merely move the docs there from the existing locations, and add javadoc references pointing to the builder from the existing fields/enum values as desired. Great. I added a unit test (just for demo) and a class `SubUnifiedHighlighter` in `TestUnifiedHighlighter.java` where I've added a new test field and also tested it. It does look right to me since it is able to use the new field and also fields from parent class. -- 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-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
