Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Commons Lang dependency #607

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ppkarwasz
Copy link

Commons Compress version 1.26.0 depends on Commons Lang, which is a 650 KiB dependency. In reality, however, only 4 methods accounting for less than 1% of Commons Lang are used.

We replace the dependency on commons-lang3 with a local copy of these 4 methods.

Commons Compress version 1.26.0 depends on Commons Lang, which is a 650 KiB dependency.
In reality, however, only 4 methods accounting for less than 1% of Commons Lang are used.

We replace the dependency on `commons-lang3` with a local copy of these 4 methods.
@garydgregory
Copy link
Member

As I've pointed out on the dev mailing list: This a horrible mess IMO. To make things even worse, this PR adds public code (the package name is irrelevant), with ZERO Javadoc and ZERO unit tests. This is not the example we should set for contributors and even less for ourselves. This seems to me like a reflection of short term hacking without long term care. I am -1, but it seems pointless as some people are in love with copy-paste over long term maintenance.

@ppkarwasz ppkarwasz marked this pull request as draft November 18, 2024 13:01
@ppkarwasz
Copy link
Author

To make things even worse, this PR adds public code (the package name is irrelevant), with ZERO Javadoc and ZERO unit tests.

Sure, there is work to be done on this PR (I marked it as draft). Don't worry, if you are willing to accept this solution, I will make the package really private (in OSGi and JPMS) and add the relevant Javadoc and unit tests.

This is not the example we should set for contributors and even less for ourselves. This seems to me like a reflection of short term hacking without long term care. I am -1, but it seems pointless as some people are in love with copy-paste over long term maintenance.

Let's ignore FieldUtils for now. What kind of maintenance work do you think this code will require in the following years? As I see it:

  • The usages of ArrayFill in Commons Compress are overkill: all the usages provide a non-null array parameter, so the method can be simplified to:
    public int[] fill(int[] a, int val) {
      Arrays.fill(a, val);
      return a;
    }
    Also note that Commons Lang does not provide a helper method for the most common usage of Arrays.fill, which is:
    public int[] createFilledArray(int size, int val) {
      int[] a = new int[size];
      Arrays.fill(a, val);
      return a;
    }
  • The implementation of SystemProperties.getOsName() in Commons Lang delegates to a complex wrapper to System.getProperty(name) that handles security exceptions and an empty or null parameter name. None of these conditions apply to the os.name property, which must have a valuet (see Javadoc for System.getProperties() and should be allowed by SecurityManager.

FieldUtils

The code that relies on FieldUtils constitutes a maintenance problem itself, since it relies on the protected FilterInputStream.in field. The field is protected for a reason: users of FilterInputStream should not skip access the underlying stream directly, but use the wrapping FilterInputStream instead.

@Glavo
Copy link
Contributor

Glavo commented Dec 3, 2024

I released Kala Compress 1.27.1-1. It is based on commons-compress, but does not depend on commons-io and commons-lang. Maybe this will be what you want.

@ppkarwasz
Copy link
Author

I released Kala Compress 1.27.1-1. It is based on commons-compress, but does not depend on commons-io and commons-lang. Maybe this will be what you want.

I don't believe that forking Commons Compress is the right approach. Your changes in Kala Compress are aligned with what we plan for Commons Compress 2.x.

I have created a 2.x branch. Feel free to open PRs against that branch to split Commons Compress in separate modules. Probably the first module we need to split is a commons-compress2-api module that would include the interfaces to access the various compression algorithms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants