[TEXT] Remove Commons Lang Dependency?

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[TEXT] Remove Commons Lang Dependency?

Pascal Schumacher
Hello everybody,

commons-text currently depends on commons-lang. This dependency is
included into the jar via the maven shade plugin.

Only two methods from lang are really needed:

StringUtils#containsAny

StringUtils#containsNone

As dependencies should be minimized I'm not sure if adding a dependency
for two methods (less than 60 lines of code) is a good idea.

What do you think?

-Pascal

P.S.: I have created a pull request:
https://github.com/apache/commons-text/pull/18/files to demonstrate the
changes necessary for the removal.







---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [TEXT] Remove Commons Lang Dependency?

Matt Sicker
That looks good to me. We do similarly in Log4j. Commons classes tend to be
written in such a way that you can copy/paste only what you need if you're
trying to prevent required dependencies.

On 18 December 2016 at 12:06, Pascal Schumacher <[hidden email]>
wrote:

> Hello everybody,
>
> commons-text currently depends on commons-lang. This dependency is
> included into the jar via the maven shade plugin.
>
> Only two methods from lang are really needed:
>
> StringUtils#containsAny
>
> StringUtils#containsNone
>
> As dependencies should be minimized I'm not sure if adding a dependency
> for two methods (less than 60 lines of code) is a good idea.
>
> What do you think?
>
> -Pascal
>
> P.S.: I have created a pull request: https://github.com/apache/comm
> ons-text/pull/18/files to demonstrate the changes necessary for the
> removal.
>
>
>
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


--
Matt Sicker <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [TEXT] Remove Commons Lang Dependency?

sebb-2-2
Looks OK.
However it might be confusing to have the same methods in both jars,
so at least the class needs to be renamed.

Probably best to make the methods internal.
Otherwise people might expect other related methods to be supported.
This would end up with duplication of LANG methods.

If it is later decided the TEXT should support more of such methods
they can be made external at that point.

On 18 December 2016 at 18:10, Matt Sicker <[hidden email]> wrote:

> That looks good to me. We do similarly in Log4j. Commons classes tend to be
> written in such a way that you can copy/paste only what you need if you're
> trying to prevent required dependencies.
>
> On 18 December 2016 at 12:06, Pascal Schumacher <[hidden email]>
> wrote:
>
>> Hello everybody,
>>
>> commons-text currently depends on commons-lang. This dependency is
>> included into the jar via the maven shade plugin.
>>
>> Only two methods from lang are really needed:
>>
>> StringUtils#containsAny
>>
>> StringUtils#containsNone
>>
>> As dependencies should be minimized I'm not sure if adding a dependency
>> for two methods (less than 60 lines of code) is a good idea.
>>
>> What do you think?
>>
>> -Pascal
>>
>> P.S.: I have created a pull request: https://github.com/apache/comm
>> ons-text/pull/18/files to demonstrate the changes necessary for the
>> removal.
>>
>>
>>
>>
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>
>
> --
> Matt Sicker <[hidden email]>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [TEXT] Remove Commons Lang Dependency?

Jörg Schaible
In reply to this post by Pascal Schumacher
Pascal Schumacher wrote:

> Hello everybody,
>
> commons-text currently depends on commons-lang. This dependency is
> included into the jar via the maven shade plugin.
>
> Only two methods from lang are really needed:
>
> StringUtils#containsAny
>
> StringUtils#containsNone
>
> As dependencies should be minimized I'm not sure if adding a dependency
> for two methods (less than 60 lines of code) is a good idea.
>
> What do you think?

Actually I don't understand why do you want to remove it. It was made
dependent on purpose. The shade plugin - if properly configured - will only
include StringUtils as a private package in commons-text - so this
dependency exists only at build time. We had a bad history of copied code in
commons, because the original code got bug fixes and the copies were never
updated later on.

Cheers,
Jörg


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [TEXT] Remove Commons Lang Dependency?

sebb-2-2
On 18 December 2016 at 22:02, Jörg Schaible <[hidden email]> wrote:

> Pascal Schumacher wrote:
>
>> Hello everybody,
>>
>> commons-text currently depends on commons-lang. This dependency is
>> included into the jar via the maven shade plugin.
>>
>> Only two methods from lang are really needed:
>>
>> StringUtils#containsAny
>>
>> StringUtils#containsNone
>>
>> As dependencies should be minimized I'm not sure if adding a dependency
>> for two methods (less than 60 lines of code) is a good idea.
>>
>> What do you think?
>
> Actually I don't understand why do you want to remove it. It was made
> dependent on purpose. The shade plugin - if properly configured - will only
> include StringUtils as a private package in commons-text - so this
> dependency exists only at build time. We had a bad history of copied code in
> commons, because the original code got bug fixes and the copies were never
> updated later on.

In which case what needs to be done is to document this decision -
e.g. in the pom - so future maintainers don't have to go through this
again.

> Cheers,
> Jörg
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [TEXT] Remove Commons Lang Dependency?

Bruno P. Kinoshita-3
In reply to this post by Pascal Schumacher
Hi Pascal,

I suspect we may need a few other methods in the future, but for now
I agree we are not using much of [lang] in [text].

If you would like to release 1.0 without the [lang] dependency, and then
we review it later whether we include it or not, I'd be fine with that.
Though I'd be happy with a 1.0 release with shaded methods too. So I
believe my vote would be a +0 :)

Hope that helps
Bruno




----- Original Message -----

> From: Pascal Schumacher <[hidden email]>
> To: Commons Developers List <[hidden email]>
> Sent: Monday, 19 December 2016 7:06 AM
> Subject: [TEXT] Remove Commons Lang Dependency?
>
> Hello everybody,
>
> commons-text currently depends on commons-lang. This dependency is
> included into the jar via the maven shade plugin.
>
> Only two methods from lang are really needed:
>
> StringUtils#containsAny
>
> StringUtils#containsNone
>
> As dependencies should be minimized I'm not sure if adding a dependency
> for two methods (less than 60 lines of code) is a good idea.
>
> What do you think?
>
> -Pascal
>
> P.S.: I have created a pull request:
> https://github.com/apache/commons-text/pull/18/files to demonstrate the
> changes necessary for the removal.
>
>
>
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [TEXT] Remove Commons Lang Dependency?

Rob Tompkins


> On Dec 18, 2016, at 5:08 PM, Bruno P. Kinoshita <[hidden email]> wrote:
>
> Hi Pascal,
>
> I suspect we may need a few other methods in the future, but for now
> I agree we are not using much of [lang] in [text].
>
> If you would like to release 1.0 without the [lang] dependency, and then
> we review it later whether we include it or not, I'd be fine with that.
> Though I'd be happy with a 1.0 release with shaded methods too. So I
> believe my vote would be a +0 :)
>

I'm a +0 here as well. Feels like fewer dependencies is a better practice, but I'm ok with including now or down the road.

-Rob

> Hope that helps
> Bruno
>
>
>
>
> ----- Original Message -----
>> From: Pascal Schumacher <[hidden email]>
>> To: Commons Developers List <[hidden email]>
>> Sent: Monday, 19 December 2016 7:06 AM
>> Subject: [TEXT] Remove Commons Lang Dependency?
>>
>> Hello everybody,
>>
>> commons-text currently depends on commons-lang. This dependency is
>> included into the jar via the maven shade plugin.
>>
>> Only two methods from lang are really needed:
>>
>> StringUtils#containsAny
>>
>> StringUtils#containsNone
>>
>> As dependencies should be minimized I'm not sure if adding a dependency
>> for two methods (less than 60 lines of code) is a good idea.
>>
>> What do you think?
>>
>> -Pascal
>>
>> P.S.: I have created a pull request:
>> https://github.com/apache/commons-text/pull/18/files to demonstrate the
>> changes necessary for the removal.
>>
>>
>>
>>
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [TEXT] Remove Commons Lang Dependency?

Pascal Schumacher
In reply to this post by Jörg Schaible
Am 18.12.2016 um 23:02 schrieb Jörg Schaible:
> Actually I don't understand why do you want to remove it. It was made
> dependent on purpose. The shade plugin - if properly configured - will only
> include StringUtils as a private package in commons-text - so this
> dependency exists only at build time. We had a bad history of copied code in
> commons, because the original code got bug fixes and the copies were never
> updated later on.
Well the minimizing of jar by shade does not really work, maybe the
configuration is wrong? Even if commons-lang usage is reduced to just
StringUtils (https://github.com/apache/commons-text/pull/19/files) the
shade plugin still includes a lot of classes from lang (145 KB).

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [TEXT] Remove Commons Lang Dependency?

Pascal Schumacher
Am 19.12.2016 um 11:25 schrieb Pascal Schumacher:

> Am 18.12.2016 um 23:02 schrieb Jörg Schaible:
>> Actually I don't understand why do you want to remove it. It was made
>> dependent on purpose. The shade plugin - if properly configured -
>> will only
>> include StringUtils as a private package in commons-text - so this
>> dependency exists only at build time. We had a bad history of copied
>> code in
>> commons, because the original code got bug fixes and the copies were
>> never
>> updated later on.
> Well the minimizing of jar by shade does not really work, maybe the
> configuration is wrong? Even if commons-lang usage is reduced to just
> StringUtils (https://github.com/apache/commons-text/pull/19/files) the
> shade plugin still includes a lot of classes from lang (145 KB).

Sorry, I have to correct myself. The minimizing does work, StringUtils
has (transitive) dependencies on half of lang.



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [TEXT] Remove Commons Lang Dependency?

Jörg Schaible
Pascal Schumacher wrote:

> Am 19.12.2016 um 11:25 schrieb Pascal Schumacher:
>> Am 18.12.2016 um 23:02 schrieb Jörg Schaible:
>>> Actually I don't understand why do you want to remove it. It was made
>>> dependent on purpose. The shade plugin - if properly configured -
>>> will only
>>> include StringUtils as a private package in commons-text - so this
>>> dependency exists only at build time. We had a bad history of copied
>>> code in
>>> commons, because the original code got bug fixes and the copies were
>>> never
>>> updated later on.
>> Well the minimizing of jar by shade does not really work, maybe the
>> configuration is wrong? Even if commons-lang usage is reduced to just
>> StringUtils (https://github.com/apache/commons-text/pull/19/files) the
>> shade plugin still includes a lot of classes from lang (145 KB).
>
> Sorry, I have to correct myself. The minimizing does work, StringUtils
> has (transitive) dependencies on half of lang.

OK, *that's* unfortunate. :-/

Cheers,
Jörg


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]