On Sun, Feb 11, 2018 at 8:12 PM, Chas Honton <c...@honton.org> wrote:
> Definitely create interfaces. > I agree 100% and will proceed. I thought about it overnight and it does not make sense to leave a mix of abstract classes and interfaces in StrSubstitutor. Gary > > 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 > >
