Lucene - Core
  1. Lucene - Core
  2. LUCENE-6958

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

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major 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
        16 kB
        Uwe Schindler
      2. LUCENE-6958.patch
        14 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Uwe Schindler added a comment -

          Patch.

          Show
          Uwe Schindler added a comment - Patch.
          Hide
          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
          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
          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
          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
          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
          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
          Adrien Grand added a comment -

          +1

          Show
          Adrien Grand added a comment - +1
          Hide
          Shai Erera added a comment -

          +1 to commit. Thanks Uwe!

          Show
          Shai Erera added a comment - +1 to commit. Thanks Uwe!
          Hide
          Uwe Schindler added a comment -

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

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

          New patch with better exception handling based on linked issue.

          Show
          Uwe Schindler added a comment - New patch with better exception handling based on linked issue.
          Hide
          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
          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
          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
          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

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development