[MNG-6825] Get rid of commons-lang #1080

gnodet · 2023-04-04T20:07:58Z

https://issues.apache.org/jira/browse/MNG-7756 and https://issues.apache.org/jira/browse/MNG-6825

elharo

I like a lot of this very much. I do want to avoid introducing yet more utils classes though.

maven-artifact/src/main/java/org/apache/maven/artifact/ArtifactUtils.java

maven-core/src/main/java/org/apache/maven/internal/StringUtils.java

elharo · 2023-04-04T20:36:57Z

maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java

@@ -158,7 +158,7 @@ public void testCalculateDegreeOfConcurrency() {
        int cpus = Runtime.getRuntime().availableProcessors();
        assertEquals((int) (cpus * 2.2), cli.calculateDegreeOfConcurrency("2.2C"));
        assertEquals(1, cli.calculateDegreeOfConcurrency("0.0001C"));
-        assertThrows(IllegalArgumentException.class, () -> cli.calculateDegreeOfConcurrency("2.C"));
+        assertThrows(IllegalArgumentException.class, () -> cli.calculateDegreeOfConcurrency("2.xC"));


I don't understand why this changed here

When changing the related code, I found out that 2.C was rejected early, although fully supported, so the test ended up failing after removing the early test which was using commons-lang Validate, as this part is completely duplicated by the exception thrown by the Float.parseFloat which also validates the syntax.

OK, so it's an extra test. That's fine, but I suggest also keeping

assertThrows(IllegalArgumentException.class, () -> cli.calculateDegreeOfConcurrency("2.C"));

It's not really an extra test per se, I saw it as a test checking a bad float syntax. That's still the case.

I can't keep really the old test, because that test would not throw an exception anymore, as 2. is a legit float number according to the Float.parseFloat which is now used to validate the syntax. So while it is of course doable, it would actually add some specific code in the calculateDegreeOfConcurrentcy method simply to ensure this test fails. I'm not really struck by the fact that 2. is correctly parsed, and therefore that 2.C is a valid degree of concurrency, so I'd rather keep the code lean and accept this use case as valid.

So this is a behavior change and a bug fix? That's good, but probably needs a specific Jira issue focused on this particularly, and an individual PR. Otherwise if this behavior change causes problems it's going to be quite hard to backtrack to this change in the middle of all the other refactoring.

It's a behaviour change for sure, whether this is considered a bug can be discussed I suppose. I'll raise a JIRA for that one and rebase on top of it.

I've splitted in two commits (not to be squashed) and two JIRA issues...

timtebeek · 2023-04-05T10:52:51Z

Great to see these changes! I think the is(Not)Empty cases to move away from utils can also be reflected in an automated migration if you'd like me to roll that out across all of Maven. I think that was the most common use of utils.

Other cases that came up in discussion elsewhere are not as clear in the general case, but can be automated in specific cases. For instance the utils split methods take a separator as a literal, rather than the regex it can be for String.split. We can explore what we can replace with JDK calls already, now, and I'll hold off on further Commons Lang changes while we do so.

I'm traveling this week, so can't fully dive in, but hope to revisit the automations next week. Let me know if there's any over replacements that you'd like to distill from this one. Only looking to help & amplify changes such as this PR.

gnodet · 2023-04-05T10:59:32Z

Great to see these changes! I think the is(Not)Empty cases to move away from utils can also be reflected in an automated migration if you'd like me to roll that out across all of Maven. I think that was the most common use of utils.

Other cases that came up in discussion elsewhere are not as clear in the general case, but can be automated in specific cases. For instance the utils split methods take a separator as a literal, rather than the regex it can be for String.split. We can explore what we can replace with JDK calls already, now, and I'll hold off on further Commons Lang changes while we do so.

I'm traveling this week, so can't fully dive in, but hope to revisit the automations next week. Let me know if there's any over replacements that you'd like to distill from this one. Only looking to help & amplify changes such as this PR.

Yes, the split calls can be translated directly when the separator is a one-char string. Other cases may need to be handled differently.

elharo

Looking good. The behavior change in MavenCli perhaps needs to be split out to a separate PR/issue though.

maven-core/src/main/java/org/apache/maven/rtinfo/internal/DefaultRuntimeInformation.java

maven-core/src/test/java/org/apache/maven/artifact/handler/ArtifactHandlerTest.java

elharo · 2023-04-05T10:58:56Z

maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java

@@ -158,7 +158,7 @@ public void testCalculateDegreeOfConcurrency() {
        int cpus = Runtime.getRuntime().availableProcessors();
        assertEquals((int) (cpus * 2.2), cli.calculateDegreeOfConcurrency("2.2C"));
        assertEquals(1, cli.calculateDegreeOfConcurrency("0.0001C"));
-        assertThrows(IllegalArgumentException.class, () -> cli.calculateDegreeOfConcurrency("2.C"));
+        assertThrows(IllegalArgumentException.class, () -> cli.calculateDegreeOfConcurrency("2.xC"));


So this is a behavior change and a bug fix? That's good, but probably needs a specific Jira issue focused on this particularly, and an individual PR. Otherwise if this behavior change causes problems it's going to be quite hard to backtrack to this change in the middle of all the other refactoring.

Also fix the thrown exception to report valid syntaxes without assuming the user intent (for example "0.5c" would say it's not an int...)

elharo

Looking good. Just a few nits.

elharo · 2023-04-05T17:48:27Z

maven-artifact/pom.xml

-      <artifactId>commons-lang3</artifactId>
-    </dependency>
-  </dependencies>
+  <dependencies />


do we need the empty element or can we just drop it?

elharo · 2023-04-05T17:50:02Z

maven-core/src/main/java/org/apache/maven/rtinfo/internal/DefaultRuntimeInformation.java

@@ -90,7 +88,9 @@ private String loadMavenVersion() {

    @Override
    public boolean isMavenVersion(String versionRange) {
-        Validate.notBlank(versionRange, "versionRange can neither be null, empty nor blank");
+        if (!(versionRange != null && !versionRange.isEmpty())) {


slightly clearer:

if (versionRange == null || versionRange.isEmpty())

elharo · 2023-04-05T17:52:14Z

maven-core/src/main/java/org/apache/maven/rtinfo/internal/DefaultRuntimeInformation.java

@@ -90,7 +88,9 @@ private String loadMavenVersion() {

    @Override
    public boolean isMavenVersion(String versionRange) {
-        Validate.notBlank(versionRange, "versionRange can neither be null, empty nor blank");
+        if (!(versionRange != null && !versionRange.isEmpty())) {
+            throw new IllegalArgumentException("versionRange can neither be null nor empty");


better to use a NullPointerException for null. The tests below suggest this was the existing behavior.

I've always disliked throwing NPE explicitly and this will complicate the code as the test need to be split... Imho, in all cases, those are illegal arguments...
I'm going to commit the following:

if (Objects.requireNonNull(versionRange, "versionRange cannot be null").isEmpty()) { throw new IllegalArgumentException("versionRange cannot be empty"); }

That works :-)

gnodet mentioned this pull request Apr 4, 2023

[MNG-6825] Remove plexus-utils and commons-xxx references apache/maven-mvnd#833

Merged

gnodet force-pushed the MNG-6825 branch 2 times, most recently from bb53cea to db428b5 Compare April 4, 2023 20:36

elharo requested changes Apr 4, 2023

View reviewed changes

gnodet force-pushed the MNG-6825 branch from e6a7d2c to d14c037 Compare April 4, 2023 21:08

gnodet requested a review from elharo April 5, 2023 07:57

elharo requested changes Apr 5, 2023

View reviewed changes

[MNG-7756] The degree of concurrency does not support "2." as a factor

aa2532a

Also fix the thrown exception to report valid syntaxes without assuming the user intent (for example "0.5c" would say it's not an int...)

gnodet force-pushed the MNG-6825 branch 2 times, most recently from b3ee1a0 to 43eac66 Compare April 5, 2023 13:58

khmarbaise approved these changes Apr 5, 2023

View reviewed changes

slachiewicz approved these changes Apr 5, 2023

View reviewed changes

gnodet requested review from elharo and khmarbaise April 5, 2023 17:32

elharo reviewed Apr 5, 2023

View reviewed changes

khmarbaise approved these changes Apr 5, 2023

View reviewed changes

gnodet force-pushed the MNG-6825 branch 2 times, most recently from deecf96 to 10a2e87 Compare April 5, 2023 18:46

gnodet requested a review from elharo April 5, 2023 18:51

[MNG-6825] Get rid of commons-lang

ed994c8

gnodet force-pushed the MNG-6825 branch from 10a2e87 to ed994c8 Compare April 5, 2023 18:54

gnodet merged commit b2ee29e into apache:master Apr 6, 2023

gnodet added this to the 4.0.0-alpha-6 milestone May 17, 2023

gnodet deleted the MNG-6825 branch November 18, 2023 21:40

cstamas mentioned this pull request Jun 6, 2024

[MNG-8146] Drop commons-lang #1564

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MNG-6825] Get rid of commons-lang #1080

[MNG-6825] Get rid of commons-lang #1080

gnodet commented Apr 4, 2023 •

edited

Loading

elharo left a comment

elharo Apr 4, 2023

gnodet Apr 4, 2023

elharo Apr 4, 2023

gnodet Apr 5, 2023 •

edited

Loading

elharo Apr 5, 2023

gnodet Apr 5, 2023

gnodet Apr 5, 2023

timtebeek commented Apr 5, 2023

gnodet commented Apr 5, 2023

elharo left a comment

elharo Apr 5, 2023

elharo left a comment

elharo Apr 5, 2023

elharo Apr 5, 2023

elharo Apr 5, 2023

gnodet Apr 5, 2023

elharo Apr 5, 2023

[MNG-6825] Get rid of commons-lang #1080

[MNG-6825] Get rid of commons-lang #1080

Conversation

gnodet commented Apr 4, 2023 • edited Loading

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnodet Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timtebeek commented Apr 5, 2023

gnodet commented Apr 5, 2023

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnodet commented Apr 4, 2023 •

edited

Loading

gnodet Apr 5, 2023 •

edited

Loading