Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-6958

Improve CustomAnalyzer to also allow to specify factory directly (for compile-time safety)

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 5.4
    • Fix Version/s: 5.5, 6.0
    • Component/s: modules/analysis
    • Labels:
      None
    • Lucene Fields:
      New
    • Flags:
      Patch

      Description

      Currently CustomAnalyzer only allows to specify the SPI names of factories. As the fluent builder pattern is mostly used inside Java code, it is better for type safety to optionally also specify the factory class directly (using compile-time safe patterns like .withTokenizer(WhitespaceTokenizerFactory.class)). With the string names, you get the error only at runtime. Of course this does not help with wrong, spelled parameter names, but it also has the side effect that you can click on the class name in your code to get javadocs with the parameter names.

      This issue will add this functionality and update the docs/example.

      Thanks to Shai Erera for suggesting this!

      1. LUCENE-6958.patch
        14 kB
        Uwe Schindler
      2. LUCENE-6958.patch
        16 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1723028 from Uwe Schindler in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1723028 ]

          Merged revision(s) 1723027 from lucene/dev/trunk:
          LUCENE-6958: Improve CustomAnalyzer to take class references to factories as alternative to their SPI name. This enables compile-time safety when defining analyzer's components

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1723028 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1723028 ] Merged revision(s) 1723027 from lucene/dev/trunk: LUCENE-6958 : Improve CustomAnalyzer to take class references to factories as alternative to their SPI name. This enables compile-time safety when defining analyzer's components
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1723027 from Uwe Schindler in branch 'dev/trunk'
          [ https://svn.apache.org/r1723027 ]

          LUCENE-6958: Improve CustomAnalyzer to take class references to factories as alternative to their SPI name. This enables compile-time safety when defining analyzer's components

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1723027 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1723027 ] LUCENE-6958 : Improve CustomAnalyzer to take class references to factories as alternative to their SPI name. This enables compile-time safety when defining analyzer's components
          Hide
          thetaphi Uwe Schindler added a comment -

          New patch with better exception handling based on linked issue.

          Show
          thetaphi Uwe Schindler added a comment - New patch with better exception handling based on linked issue.
          Hide
          thetaphi Uwe Schindler added a comment -

          Thanks Shai; I will change the patch a bit and upload a new one based on LUCENE-6961.

          Show
          thetaphi Uwe Schindler added a comment - Thanks Shai; I will change the patch a bit and upload a new one based on LUCENE-6961 .
          Hide
          shaie Shai Erera added a comment -

          +1 to commit. Thanks Uwe!

          Show
          shaie Shai Erera added a comment - +1 to commit. Thanks Uwe!
          Hide
          jpountz Adrien Grand added a comment -

          +1

          Show
          jpountz Adrien Grand added a comment - +1
          Hide
          thetaphi Uwe Schindler added a comment -

          Yes, would remove some code duplication, but that's a one-liner only. Will think about it.

          I don't think this is needed. It is only one method call and the boolean flag change. If we have further refactoring this would make sense, but now it adds 3 methods for nothing (and makes it unreadable).

          Show
          thetaphi Uwe Schindler added a comment - Yes, would remove some code duplication, but that's a one-liner only. Will think about it. I don't think this is needed. It is only one method call and the boolean flag change. If we have further refactoring this would make sense, but now it adds 3 methods for nothing (and makes it unreadable).
          Hide
          thetaphi Uwe Schindler added a comment -

          Looks good Uwe. Do you think we should delegate all the variants now to a private e.g. withTokenizer(TokenizerFactory) which will also set components added, call applyResourceLoader etc.?

          Yes, would remove some code duplication, but that's a one-liner only. Will think about it.
          CHANGES.txt will be added con commit.

          Thanks for review.

          Show
          thetaphi Uwe Schindler added a comment - Looks good Uwe. Do you think we should delegate all the variants now to a private e.g. withTokenizer(TokenizerFactory) which will also set components added, call applyResourceLoader etc.? Yes, would remove some code duplication, but that's a one-liner only. Will think about it. CHANGES.txt will be added con commit. Thanks for review.
          Hide
          shaie Shai Erera added a comment -

          Looks good Uwe. Do you think we should delegate all the variants now to a private e.g. withTokenizer(TokenizerFactory) which will also set components added, call applyResourceLoader etc.?

          Also, should you mention it in CHANGES?

          Otherwise, +1 to commit and thanks for taking care of this so quickly!

          Show
          shaie Shai Erera added a comment - Looks good Uwe. Do you think we should delegate all the variants now to a private e.g. withTokenizer(TokenizerFactory) which will also set components added, call applyResourceLoader etc.? Also, should you mention it in CHANGES? Otherwise, +1 to commit and thanks for taking care of this so quickly!
          Hide
          thetaphi Uwe Schindler added a comment -

          Patch.

          Show
          thetaphi Uwe Schindler added a comment - Patch.

            People

            • Assignee:
              thetaphi Uwe Schindler
              Reporter:
              thetaphi Uwe Schindler
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development