gnodet commented on code in PR #11284:
URL: https://github.com/apache/maven/pull/11284#discussion_r2436512513
##########
api/maven-api-xml/src/main/java/org/apache/maven/api/xml/XmlNode.java:
##########
@@ -527,7 +527,7 @@ private String toStringObject() {
w = addToStringField(sb, value, o -> !o.isEmpty(), "value", w);
w = addToStringField(sb, attributes, o -> !o.isEmpty(),
"attributes", w);
w = addToStringField(sb, children, o -> !o.isEmpty(),
"children", w);
- w = addToStringField(sb, inputLocation, Objects::nonNull,
"inputLocation", w);
+ addToStringField(sb, inputLocation, Objects::nonNull,
"inputLocation", w);
Review Comment:
I agree the last variable value is not used, but I'm really skeptical about
the benefit, and we're loosing ease of understanding imho.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java:
##########
@@ -91,27 +93,14 @@ public <T> T process(T object) {
}
private String getProperty(String name, String defaultValue) {
- Object value = properties.get(name);
- return value instanceof String str ? str : defaultValue;
- }
-
- /**
- * Gets the set of object types that should be pooled.
- */
- private Set<String> getPooledTypes(Map<?, ?> properties) {
- String pooledTypesProperty =
getProperty(Constants.MAVEN_MODEL_PROCESSOR_POOLED_TYPES, "Dependency");
- return Arrays.stream(pooledTypesProperty.split(","))
- .map(String::trim)
- .filter(s -> !s.isEmpty())
- .collect(Collectors.toSet());
+ return properties.get(name) instanceof String str ? str : defaultValue;
}
/**
* Creates a cache for the specified object type with the appropriate
reference type.
*/
private Cache<PoolKey, Object> createCacheForType(Class<?> objectType) {
- Cache.ReferenceType referenceType =
getReferenceTypeForClass(objectType);
- return Cache.newCache(referenceType);
+ return Cache.newCache(getReferenceTypeForClass(objectType));
Review Comment:
What rule of yours does the above break ?
I don't think using variables is considered a bad practice.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java:
##########
@@ -77,10 +77,12 @@ public <T> T process(T object) {
}
Class<?> objectType = object.getClass();
- String simpleClassName = objectType.getSimpleName();
Review Comment:
I'd keep this variable.
And also add one for the `pooledTypesProperty` from the (now inlined)
`getPooledTypes`. The code would be more readable imho.
##########
pom.xml:
##########
@@ -23,7 +23,7 @@ under the License.
<parent>
<groupId>org.apache.maven</groupId>
<artifactId>maven-parent</artifactId>
- <version>45</version>
+ <version>46-SNAPSHOT</version>
Review Comment:
Unrelated
##########
.mvn/jvm.config:
##########
@@ -0,0 +1,10 @@
+--add-exports jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED
Review Comment:
Unrelated
##########
impl/maven-xml/src/main/java/org/apache/maven/internal/xml/XmlNodeImpl.java:
##########
@@ -310,7 +310,7 @@ public String toStringObject() {
w = addToStringField(sb, value, o -> !o.isEmpty(), "value", w);
w = addToStringField(sb, attributes, o -> !o.isEmpty(), "attributes",
w);
w = addToStringField(sb, children, o -> !o.isEmpty(), "children", w);
- w = addToStringField(sb, location, Objects::nonNull, "location", w);
+ addToStringField(sb, location, Objects::nonNull, "location", w);
Review Comment:
no real benefit, and we're loosing some coherence, which makes the code
slightly more difficult to read.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java:
##########
@@ -142,9 +131,9 @@ private Cache.ReferenceType
getReferenceTypeForClass(Class<?> objectType) {
*/
private Cache.ReferenceType getDefaultReferenceType() {
try {
- String referenceTypeProperty =
-
getProperty(Constants.MAVEN_MODEL_PROCESSOR_REFERENCE_TYPE,
Cache.ReferenceType.HARD.name());
- return
Cache.ReferenceType.valueOf(referenceTypeProperty.toUpperCase());
+ return Cache.ReferenceType.valueOf(
Review Comment:
Same here. It's still 3 lines, and less readable.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java:
##########
@@ -228,17 +228,16 @@ private Settings readSettings(
return settings;
}
- private Settings interpolate(
- Settings settings, SettingsBuilderRequest request,
ProblemCollector<BuilderProblem> problems) {
- UnaryOperator<String> src;
- if (request.getInterpolationSource().isPresent()) {
- src = request.getInterpolationSource().get();
- } else {
- Map<String, String> userProperties =
request.getSession().getUserProperties();
- Map<String, String> systemProperties =
request.getSession().getSystemProperties();
- src = Interpolator.chain(userProperties::get,
systemProperties::get);
- }
- return new DefSettingsTransformer(value -> value != null ?
interpolator.interpolate(value, src) : null)
+ private Settings interpolate(Settings settings, SettingsBuilderRequest
request) {
Review Comment:
Once again, those are not unused variables. These are intermediary results.
Your refactoring hinders readability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
