commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <garydgreg...@gmail.com>
Subject Re: [text] Adapt the Log4j 2 Interpolator to [text]
Date Sun, 11 Feb 2018 21:57:14 GMT
On Sun, Feb 11, 2018 at 2:46 PM, Gary Gregory <garydgregory@gmail.com>
wrote:

>
>
> On Sun, Feb 11, 2018 at 12:05 PM, Pascal Schumacher <
> pascalschumacher@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-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message