Github user dsmiley commented on a diff in the pull request: https://github.com/apache/lucene-solr/pull/384#discussion_r191574085 --- Diff: lucene/analysis/common/src/java/org/apache/lucene/analysis/miscellaneous/ConcatenateGraphFilter.java --- @@ -119,27 +147,19 @@ public boolean incrementToken() throws IOException { @Override public void end() throws IOException { - super.end(); - if (finiteStrings == null) { - inputTokenStream.end(); - } - } - - @Override - public void close() throws IOException { - if (finiteStrings == null) { - inputTokenStream.close(); - } + restoreState(endState); } + //nocommit move method to before incrementToken @Override public void reset() throws IOException { - super.reset(); - if (hasAttribute(CharTermAttribute.class)) { - // we only create this if we really need it to safe the UTF-8 to UTF-16 conversion - charTermAttribute = getAttribute(CharTermAttribute.class); --- End diff -- Oh *that* comment. By that I mean move the _method_ to follow a natural sequence. I like classes to have methods that are organized sensibly. When one uses a TokenStream, reset() should come before incrementToken() ! -- then end(), then close(), and any utility methods should come after callers. I moved the initialization to reset simply because it felt natural this way rather than incrementToken where it's lazy. That's all. That said... I hinted at a TestRandomChains failure about lifecycle needing to throw an IllegalStateException or something like that (pertaining to missing close()), and I think making this back to lazy will "fix" that.
--- --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org