Prefer Java standard library to Plexus util #1015
Conversation
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java
Outdated
Show resolved
Hide resolved
...ngs-builder/src/main/java/org/apache/maven/settings/validation/DefaultSettingsValidator.java
Outdated
Show resolved
Hide resolved
| @@ -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.trim().isEmpty()) { | |||
| addViolation( | |||
| problems, Severity.ERROR, "pluginGroups.pluginGroup[" + i + "]", null, "must not be empty"); | |||
| } else if (!ID_REGEX.matcher(pluginGroup).matches()) { | |||
There was a problem hiding this comment.
This error message is misleading...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I am included to drop the entire trim. The rest of the class does not trim at all. It should be consistent throughout elements.
|
Yes, I would like to remove all references to org.codehaus.plexus.utils.StringUtils. In fact I would like to remove all references to org.codehaus.plexus.utils and now that you mention it, I would like to get rid of all references to org.codehaus.plexus. Realistically plexus only ever was used in Maven and there's no need for separate projects or repos. But even sticking to StringUtils that's a long slog across multiple repos, and even in a single repo the semantics of replacement for a call can vary subtly from one invocation to the next so I don't think a single PR is practical. |
| @@ -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.trim().isEmpty()) { | |||
| addViolation( | |||
| problems, Severity.ERROR, "pluginGroups.pluginGroup[" + i + "]", null, "must not be empty"); | |||
| } else if (!ID_REGEX.matcher(pluginGroup).matches()) { | |||
There was a problem hiding this comment.
I am included to drop the entire trim. The rest of the class does not trim at all. It should be consistent throughout elements.
...ngs-builder/src/main/java/org/apache/maven/settings/validation/DefaultSettingsValidator.java
Outdated
Show resolved
Hide resolved
Begin slog of removing plexus util dependencies we shouldn't have to maintain.