Definitely create interfaces. Chas
> On Feb 11, 2018, at 1:57 PM, Gary Gregory <garydgreg...@gmail.com> wrote: > > 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 >>> >>> >> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
