On Sun, Feb 11, 2018 at 2:46 PM, Gary Gregory <garydgreg...@gmail.com> wrote:
> > > On Sun, Feb 11, 2018 at 12:05 PM, Pascal Schumacher < > pascalschumac...@gmx.net> wrote: > >> Am 11.02.2018 um 19:24 schrieb Gary Gregory: >> >>> I'd like a code review and then a release of 1.3. Right now we only >>> depend >>> on java.base and Commons Lang, so let's keep it that way for 1.3 I think. >>> >> My comments: >> > > Thank you for the code review! > > >> - Given "TEXT-80: StrLookup API confusing generic type parameter" I think >> we should deprecate the old StrLookup class and mark it for removal in >> commons-text 2.0. >> > +1 and will do. > > >> - DateStringLookup: should we use FastDateFormat? >> > > I overlooked that one, I'll look into it. > > >> - AbstractStringLookup: empty class, I would therefore remove it. >> > > I like to have it around for now, it is package private anyway. We can > remove it before 1.3 if it stays empty. At one point I have an > isEmpty(String) method in there before I realized that Commons Text depends > on Commons Lang which provides the same service in > StringUtils.isEmpty(String). > > >> - StringLookupFactory: should this be a static factory, to make it easier >> to use? > > > I am leaving room for configuring these things later so I feel that doing > .INSTANCE is a small price to pay but I hear you. > Oh, and consider this alternative: - Create StringLookup (already there) and StringSubstitutor - Deprecate StrLookup and StrSubstitutor and leave as is from 1.2 - ALSO deprecate StrMatcher which is a class and introduce a StringMatcher _interface_. The idea here is to avoid having to do this a second time later. Right now StrLookup and StrMatcher are classes, there are no interfaces. The question is: Should we just redo the whole thing based on interfaces now. As of now in master, we have a 50/50 situation with StrSubstituor supporting both StrLookup and StringLookup AND using StrMatcher (a class, not an interface). Thoughts? Gary > Gary > > >> >> >> (I almost added Log4j's JNDI lookup but I know that will not work on >>> Android so I'd like to leave stuff like that for later, maybe in a >>> different module.) >>> >> +1 for leaving it out >> >> -Pascal >> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >> >
