[GitHub] [lucene-solr] msokolov commented on issue #892: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

msokolov commented on issue #892: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation
URL: https://github.com/apache/lucene-solr/pull/892#issuecomment-562639099
 
 
   Hi @magibney, thanks for addressing my earlier comments and waiting so long for me to get back. Just a few more things I'd like to see changed before we commit. 
   
   The constructors for the char filter are package private, so this can only be created using the factory. I think this is a kind of unusual pattern, at least I checked a couple of other CharFilter classes and they do seem to be accessible, with public constructors. I know we have consumers of the analysis code that don't use the factory abstraction to create their analysis chains, so I think it would be good to support that here too. What do you think about (1) making both ICUTransformCharFilter constructors public, (2) moving modifyRules to that class so users will think of using it, and perhaps suggesting that it would be a good idea to do so for perf reasons in javadoc of this class, and (3) testing this in the unit tests?
   
   I rebased and ran precommit locally and it passed OK: I'm not sure why we got that failure above. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org