Details
-
Type:
Improvement
-
Status: Closed
-
Priority:
Minor
-
Resolution: Fixed
-
Affects Version/s: None
-
Component/s: modules/analysis
-
Labels:None
-
Lucene Fields:New
Description
Currently WordDelimiterFilter's constructor is:
public WordDelimiterFilter(TokenStream in, byte[] charTypeTable, int generateWordParts, int generateNumberParts, int catenateWords, int catenateNumbers, int catenateAll, int splitOnCaseChange, int preserveOriginal, int splitOnNumerics, int stemEnglishPossessive, CharArraySet protWords) {
which means its instantiation is an unreadable combination of 1s and 0s.
We should improve this by either using a Builder, 'int flags' or an EnumSet.
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
For historical context, the reason I used an int for stuff like generateWordParts was that I had the idea of using it as a minimum (i.e. only generate word parts that are over a certain size, etc). This obviously never happened though 
OK, if those integers are always used only as boolean flags, I would prefer a single (int flags) parameter. No builder pattern needed. I would maybe prefer a long to make it extensibler (but 31 flags should be enough, too).
Patch for WDF & co, converting them over to int flags.
Old behavior is deprecated but we could do that in 3x and nuke in trunk.
Plan to commit soon if theres no objections.
+1
looks good overall, a couple tiny nitpicks:
- looks like there is some dead code in WordDelimiterIterator (the booleans)
- should the iterator maybe keep the booleans and not use flags? just an idea, because the iterator doesn't "make use" of all the flags. its also not a public class and just a helper class to simplify the filter, so i think its ok for it to take 3 booleans?
should the iterator maybe keep the booleans and not use flags? just an idea, because the iterator doesn't "make use" of all the flags. its also not a public class and just a helper class to simplify the filter, so i think its ok for it to take 3 booleans?
Yeah I thought about this as well. It would make the iterator clearer since it wouldn't rely on people looking at the Filter's flags. I will make the change.
Patch with the Iterator back to using booleans. Going to commit.
Better patch.
Committed revision 1165995.
any objections to backporting? we could then remove the deprecation in trunk.
None whatsoever.
I can take it if you want Chris, unless you want to do the honors?
Go for it!
backported to 3.5
Bulk close after release of 3.5
I think flags is a good solution here, its very simple and will improve readability: the backwards compat is obvious too.
I think its a bit scary to use enumset, it will involve complicated generics and the jdk itself does not seem to use enumset anywhere!
e.g. Pattern.compile(String regex, int flags)
I think a builder is overkill here, if someone wants a builder they can always make a builder on top of flags for their own use.