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.

Sign up for GitHub

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

Prefer Java standard library to Plexus util #1015

Merged
merged 8 commits into from
May 16, 2023
Merged

Prefer Java standard library to Plexus util #1015

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e4f6e16
Prefer Java standard library to Plexus util
elharo Feb 26, 2023
772a907
format
elharo Feb 26, 2023
dd91917
revert accidental test
elharo Feb 26, 2023
92ed672
address code review
elharo Feb 26, 2023
de89800
don't trim
elharo Mar 11, 2023
2b91e28
Update DefaultSettingsValidator.java
gnodet Mar 13, 2023
7170d11
Update DefaultSettingsValidator.java
gnodet Mar 13, 2023
0fa4329
Merge branch 'master' into plexus
elharo Mar 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions maven-artifact/src/main/java/org/apache/maven/artifact/DefaultArtifact.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import org.apache.maven.artifact.versioning.DefaultArtifactVersion;
import org.apache.maven.artifact.versioning.OverConstrainedVersionException;
import org.apache.maven.artifact.versioning.VersionRange;
import org.codehaus.plexus.util.StringUtils;

/**
* @author Jason van Zyl
Expand Down Expand Up @@ -172,7 +171,7 @@ public String getClassifier() {
}

public boolean hasClassifier() {
return StringUtils.isNotEmpty(classifier);
return classifier != null && !classifier.isEmpty();
}

public String getScope() {
Expand Down
4 changes: 2 additions & 2 deletions ...n-settings-builder/src/main/java/org/apache/maven/settings/merge/MavenSettingsMerger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

import org.apache.maven.settings.IdentifiableBase;
import org.apache.maven.settings.Settings;
import org.codehaus.plexus.util.StringUtils;

/**
* @author <a href="mailto:vincent.siveton@gmail.com">Vincent Siveton</a>
Expand Down Expand Up @@ -80,7 +79,8 @@ public void merge(Settings dominant, Settings recessive, String recessiveSourceL
}
}

if (StringUtils.isEmpty(dominant.getLocalRepository())) {
if (dominant.getLocalRepository() == null
|| dominant.getLocalRepository().isEmpty()) {
dominant.setLocalRepository(recessive.getLocalRepository());
}

Expand Down
3 changes: 1 addition & 2 deletions ...-builder/src/main/java/org/apache/maven/settings/validation/DefaultSettingsValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import org.apache.maven.settings.Settings;
import org.apache.maven.settings.building.SettingsProblem.Severity;
import org.apache.maven.settings.building.SettingsProblemCollector;
import org.codehaus.plexus.util.StringUtils;

/**
* @author Milos Kleint
Expand All @@ -60,7 +59,7 @@ public void validate(Settings settings, SettingsProblemCollector problems) {
for (int i = 0; i < pluginGroups.size(); i++) {
String pluginGroup = pluginGroups.get(i).trim();

if (StringUtils.isBlank(pluginGroup)) {
if (pluginGroup.isEmpty()) {
addViolation(
problems, Severity.ERROR, "pluginGroups.pluginGroup[" + i + "]", null, "must not be empty");
} else if (!ID_REGEX.matcher(pluginGroup).matches()) {
Copy link
Member

@michael-o michael-o Feb 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message is misleading...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I see how, but in any case that would be a pre-existing, unrelated issue. Please file a JIRA on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message says "empty", but test is for blank. We should check wether other spots validate and say then "empty". Those are two different things for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and they aren't clear to me either. This is a problem with all the isEmpty, isBlank, isNullOrEmpty methods that litter utility libraries across Java, not just here but in Guava and many, many other libraries. They do not agree on what "empty", "blank", and similar terms mean. Sometimes "empty" strings can contain whitespace. Sometimes they can't. Sometimes they can be null. Sometimes they can't. It depends on which version of which library you're using. It's confusing and error prone. That's a big reason I want to get rid of all of them and stick exclusively to JDK methods.

In this case the string was already trimmed in line 60 so empty and blank are the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am included to drop the entire trim. The rest of the class does not trim at all. It should be consistent throughout elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, trim dropped

Expand Down