elharo commented on code in PR #2029:
URL: https://github.com/apache/maven/pull/2029#discussion_r1905374218


##########
api/maven-api-xml/src/main/java/org/apache/maven/api/xml/XmlNode.java:
##########
@@ -18,121 +18,244 @@
  */
 package org.apache.maven.api.xml;
 
+import javax.xml.stream.XMLStreamException;
+
+import java.io.Serializable;
+import java.io.StringWriter;
 import java.util.List;
+import java.util.ListIterator;
 import java.util.Map;
-
-import org.apache.maven.api.annotations.Experimental;
-import org.apache.maven.api.annotations.Immutable;
-import org.apache.maven.api.annotations.Nonnull;
-import org.apache.maven.api.annotations.Nullable;
-import org.apache.maven.api.annotations.ThreadSafe;
+import java.util.Objects;
+import java.util.function.Function;
 
 /**
- * An immutable xml node.
- *
- * @since 4.0.0
+ *  NOTE: remove all the util code in here when separated, this class should 
be pure data.
  */
-@Experimental
-@ThreadSafe
-@Immutable
-public interface XmlNode {
+public class XmlNode implements Serializable {
+    private static final long serialVersionUID = 2567894443061173996L;
 
-    String CHILDREN_COMBINATION_MODE_ATTRIBUTE = "combine.children";
+    public static final String CHILDREN_COMBINATION_MODE_ATTRIBUTE = 
"combine.children";
 
-    String CHILDREN_COMBINATION_MERGE = "merge";
+    public static final String CHILDREN_COMBINATION_MERGE = "merge";
 
-    String CHILDREN_COMBINATION_APPEND = "append";
+    public static final String CHILDREN_COMBINATION_APPEND = "append";
 
     /**
      * This default mode for combining children DOMs during merge means that 
where element names match, the process will
      * try to merge the element data, rather than putting the dominant and 
recessive elements (which share the same
      * element name) as siblings in the resulting DOM.
      */
-    String DEFAULT_CHILDREN_COMBINATION_MODE = CHILDREN_COMBINATION_MERGE;
+    public static final String DEFAULT_CHILDREN_COMBINATION_MODE = 
CHILDREN_COMBINATION_MERGE;
 
-    String SELF_COMBINATION_MODE_ATTRIBUTE = "combine.self";
+    public static final String SELF_COMBINATION_MODE_ATTRIBUTE = 
"combine.self";
 
-    String SELF_COMBINATION_OVERRIDE = "override";
+    public static final String SELF_COMBINATION_OVERRIDE = "override";
 
-    String SELF_COMBINATION_MERGE = "merge";
+    public static final String SELF_COMBINATION_MERGE = "merge";
 
-    String SELF_COMBINATION_REMOVE = "remove";
+    public static final String SELF_COMBINATION_REMOVE = "remove";
 
     /**
      * In case of complex XML structures, combining can be done based on id.
      */
-    String ID_COMBINATION_MODE_ATTRIBUTE = "combine.id";
+    public static final String ID_COMBINATION_MODE_ATTRIBUTE = "combine.id";
 
     /**
      * In case of complex XML structures, combining can be done based on keys.
      * This is a comma separated list of attribute names.
      */
-    String KEYS_COMBINATION_MODE_ATTRIBUTE = "combine.keys";
+    public static final String KEYS_COMBINATION_MODE_ATTRIBUTE = 
"combine.keys";
 
     /**
      * This default mode for combining a DOM node during merge means that 
where element names match, the process will
      * try to merge the element attributes and values, rather than overriding 
the recessive element completely with the
      * dominant one. This means that wherever the dominant element doesn't 
provide the value or a particular attribute,
      * that value or attribute will be set from the recessive DOM node.
      */
-    String DEFAULT_SELF_COMBINATION_MODE = SELF_COMBINATION_MERGE;
+    public static final String DEFAULT_SELF_COMBINATION_MODE = 
SELF_COMBINATION_MERGE;
+
+    protected final String prefix;
+
+    protected final String namespaceUri;
+
+    protected final String name;
+
+    protected final String value;
+
+    protected final Map<String, String> attributes;
+
+    protected final List<XmlNode> children;
+
+    protected final Object location;
+
+    public XmlNode(String name) {

Review Comment:
   needs javadoc



##########
api/maven-api-xml/src/main/java/org/apache/maven/api/xml/XmlNode.java:
##########
@@ -18,121 +18,244 @@
  */
 package org.apache.maven.api.xml;
 
+import javax.xml.stream.XMLStreamException;
+
+import java.io.Serializable;
+import java.io.StringWriter;
 import java.util.List;
+import java.util.ListIterator;
 import java.util.Map;
-
-import org.apache.maven.api.annotations.Experimental;
-import org.apache.maven.api.annotations.Immutable;
-import org.apache.maven.api.annotations.Nonnull;
-import org.apache.maven.api.annotations.Nullable;
-import org.apache.maven.api.annotations.ThreadSafe;
+import java.util.Objects;
+import java.util.function.Function;
 
 /**
- * An immutable xml node.
- *
- * @since 4.0.0
+ *  NOTE: remove all the util code in here when separated, this class should 
be pure data.
  */
-@Experimental
-@ThreadSafe
-@Immutable
-public interface XmlNode {
+public class XmlNode implements Serializable {
+    private static final long serialVersionUID = 2567894443061173996L;
 
-    String CHILDREN_COMBINATION_MODE_ATTRIBUTE = "combine.children";
+    public static final String CHILDREN_COMBINATION_MODE_ATTRIBUTE = 
"combine.children";
 
-    String CHILDREN_COMBINATION_MERGE = "merge";
+    public static final String CHILDREN_COMBINATION_MERGE = "merge";
 
-    String CHILDREN_COMBINATION_APPEND = "append";
+    public static final String CHILDREN_COMBINATION_APPEND = "append";
 
     /**
      * This default mode for combining children DOMs during merge means that 
where element names match, the process will
      * try to merge the element data, rather than putting the dominant and 
recessive elements (which share the same
      * element name) as siblings in the resulting DOM.
      */
-    String DEFAULT_CHILDREN_COMBINATION_MODE = CHILDREN_COMBINATION_MERGE;
+    public static final String DEFAULT_CHILDREN_COMBINATION_MODE = 
CHILDREN_COMBINATION_MERGE;
 
-    String SELF_COMBINATION_MODE_ATTRIBUTE = "combine.self";
+    public static final String SELF_COMBINATION_MODE_ATTRIBUTE = 
"combine.self";
 
-    String SELF_COMBINATION_OVERRIDE = "override";
+    public static final String SELF_COMBINATION_OVERRIDE = "override";
 
-    String SELF_COMBINATION_MERGE = "merge";
+    public static final String SELF_COMBINATION_MERGE = "merge";
 
-    String SELF_COMBINATION_REMOVE = "remove";
+    public static final String SELF_COMBINATION_REMOVE = "remove";
 
     /**
      * In case of complex XML structures, combining can be done based on id.
      */
-    String ID_COMBINATION_MODE_ATTRIBUTE = "combine.id";
+    public static final String ID_COMBINATION_MODE_ATTRIBUTE = "combine.id";
 
     /**
      * In case of complex XML structures, combining can be done based on keys.
      * This is a comma separated list of attribute names.
      */
-    String KEYS_COMBINATION_MODE_ATTRIBUTE = "combine.keys";
+    public static final String KEYS_COMBINATION_MODE_ATTRIBUTE = 
"combine.keys";
 
     /**
      * This default mode for combining a DOM node during merge means that 
where element names match, the process will
      * try to merge the element attributes and values, rather than overriding 
the recessive element completely with the
      * dominant one. This means that wherever the dominant element doesn't 
provide the value or a particular attribute,
      * that value or attribute will be set from the recessive DOM node.
      */
-    String DEFAULT_SELF_COMBINATION_MODE = SELF_COMBINATION_MERGE;
+    public static final String DEFAULT_SELF_COMBINATION_MODE = 
SELF_COMBINATION_MERGE;
+
+    protected final String prefix;
+
+    protected final String namespaceUri;
+
+    protected final String name;
+
+    protected final String value;
+
+    protected final Map<String, String> attributes;
+
+    protected final List<XmlNode> children;
+
+    protected final Object location;
+
+    public XmlNode(String name) {
+        this(name, null, null, null, null);
+    }
+
+    public XmlNode(String name, String value) {
+        this(name, value, null, null, null);
+    }
+
+    public XmlNode(XmlNode from, String name) {
+        this(name, from.getValue(), from.getAttributes(), from.getChildren(), 
from.getInputLocation());
+    }
+
+    public XmlNode(String name, String value, Map<String, String> attributes, 
List<XmlNode> children, Object location) {
+        this("", "", name, value, attributes, children, location);
+    }
+
+    public XmlNode(
+            String prefix,
+            String namespaceUri,
+            String name,
+            String value,
+            Map<String, String> attributes,
+            List<XmlNode> children,
+            Object location) {
+        this.prefix = prefix == null ? "" : prefix;
+        this.namespaceUri = namespaceUri == null ? "" : namespaceUri;
+        this.name = Objects.requireNonNull(name);
+        this.value = value;
+        this.attributes = ImmutableCollections.copy(attributes);
+        this.children = ImmutableCollections.copy(children);
+        this.location = location;
+    }
+    // ----------------------------------------------------------------------
+    // Name handling
+    // ----------------------------------------------------------------------
+
+    public String getPrefix() {

Review Comment:
   Javadoc on all public methods, and thi9s should be more than simply "Returns 
prefix". It should say what a prefix is.



##########
api/maven-api-xml/src/main/java/org/apache/maven/api/xml/XmlNode.java:
##########
@@ -18,121 +18,244 @@
  */
 package org.apache.maven.api.xml;
 
+import javax.xml.stream.XMLStreamException;
+
+import java.io.Serializable;
+import java.io.StringWriter;
 import java.util.List;
+import java.util.ListIterator;
 import java.util.Map;
-
-import org.apache.maven.api.annotations.Experimental;
-import org.apache.maven.api.annotations.Immutable;
-import org.apache.maven.api.annotations.Nonnull;
-import org.apache.maven.api.annotations.Nullable;
-import org.apache.maven.api.annotations.ThreadSafe;
+import java.util.Objects;
+import java.util.function.Function;
 
 /**
- * An immutable xml node.
- *
- * @since 4.0.0
+ *  NOTE: remove all the util code in here when separated, this class should 
be pure data.
  */
-@Experimental
-@ThreadSafe
-@Immutable
-public interface XmlNode {
+public class XmlNode implements Serializable {

Review Comment:
   This is exactly what I don't what to do. That just makes the API more 
complex than it needs to be.



##########
api/maven-api-xml/src/main/java/org/apache/maven/api/xml/XmlNode.java:
##########
@@ -18,121 +18,244 @@
  */
 package org.apache.maven.api.xml;
 
+import javax.xml.stream.XMLStreamException;
+
+import java.io.Serializable;
+import java.io.StringWriter;
 import java.util.List;
+import java.util.ListIterator;
 import java.util.Map;
-
-import org.apache.maven.api.annotations.Experimental;
-import org.apache.maven.api.annotations.Immutable;
-import org.apache.maven.api.annotations.Nonnull;
-import org.apache.maven.api.annotations.Nullable;
-import org.apache.maven.api.annotations.ThreadSafe;
+import java.util.Objects;
+import java.util.function.Function;
 
 /**
- * An immutable xml node.
- *
- * @since 4.0.0
+ *  NOTE: remove all the util code in here when separated, this class should 
be pure data.
  */
-@Experimental
-@ThreadSafe
-@Immutable
-public interface XmlNode {
+public class XmlNode implements Serializable {

Review Comment:
   remove Serializable



##########
impl/maven-xml/src/main/java/org/apache/maven/internal/xml/XmlNodeUtil.java:
##########
@@ -18,160 +18,28 @@
  */
 package org.apache.maven.internal.xml;
 
-import javax.xml.stream.XMLStreamException;
-
-import java.io.Serializable;
-import java.io.StringWriter;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
-import java.util.ListIterator;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
-import java.util.function.Function;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import org.apache.maven.api.xml.XmlNode;
 
-/**
- *  NOTE: remove all the util code in here when separated, this class should 
be pure data.
- */
-public class XmlNodeImpl implements Serializable, XmlNode {
-    private static final long serialVersionUID = 2567894443061173996L;
-
-    protected final String prefix;
-
-    protected final String namespaceUri;
-
-    protected final String name;
-
-    protected final String value;
-
-    protected final Map<String, String> attributes;
-
-    protected final List<XmlNode> children;
-
-    protected final Object location;
-
-    public XmlNodeImpl(String name) {
-        this(name, null, null, null, null);
-    }
-
-    public XmlNodeImpl(String name, String value) {
-        this(name, value, null, null, null);
-    }
-
-    public XmlNodeImpl(XmlNode from, String name) {
-        this(name, from.getValue(), from.getAttributes(), from.getChildren(), 
from.getInputLocation());
-    }
-
-    public XmlNodeImpl(
-            String name, String value, Map<String, String> attributes, 
List<XmlNode> children, Object location) {
-        this("", "", name, value, attributes, children, location);
-    }
-
-    public XmlNodeImpl(
-            String prefix,
-            String namespaceUri,
-            String name,
-            String value,
-            Map<String, String> attributes,
-            List<XmlNode> children,
-            Object location) {
-        this.prefix = prefix == null ? "" : prefix;
-        this.namespaceUri = namespaceUri == null ? "" : namespaceUri;
-        this.name = Objects.requireNonNull(name);
-        this.value = value;
-        this.attributes = ImmutableCollections.copy(attributes);
-        this.children = ImmutableCollections.copy(children);
-        this.location = location;
-    }
-
-    @Override
-    public XmlNode merge(XmlNode source, Boolean childMergeOverride) {
-        return merge(this, source, childMergeOverride);
-    }
-
-    // ----------------------------------------------------------------------
-    // Name handling
-    // ----------------------------------------------------------------------
-
-    @Override
-    public String getPrefix() {
-        return prefix;
-    }
-
-    @Override
-    public String getNamespaceUri() {
-        return namespaceUri;
-    }
-
-    @Override
-    public String getName() {
-        return name;
-    }
-
-    // ----------------------------------------------------------------------
-    // Value handling
-    // ----------------------------------------------------------------------
-
-    public String getValue() {
-        return value;
-    }
-
-    // ----------------------------------------------------------------------
-    // Attribute handling
-    // ----------------------------------------------------------------------
-
-    @Override
-    public Map<String, String> getAttributes() {
-        return attributes;
-    }
-
-    public String getAttribute(String name) {
-        return attributes.get(name);
-    }
-
-    // ----------------------------------------------------------------------
-    // Child handling
-    // ----------------------------------------------------------------------
-
-    public XmlNode getChild(String name) {
-        if (name != null) {
-            ListIterator<XmlNode> it = children.listIterator(children.size());
-            while (it.hasPrevious()) {
-                XmlNode child = it.previous();
-                if (name.equals(child.getName())) {
-                    return child;
-                }
-            }
-        }
-        return null;
-    }
-
-    public List<XmlNode> getChildren() {
-        return children;
-    }
-
-    public int getChildCount() {
-        return children.size();
-    }
-
-    // ----------------------------------------------------------------------
-    // Input location handling
-    // ----------------------------------------------------------------------
+import static org.apache.maven.api.xml.XmlNode.CHILDREN_COMBINATION_APPEND;
+import static 
org.apache.maven.api.xml.XmlNode.CHILDREN_COMBINATION_MODE_ATTRIBUTE;
+import static org.apache.maven.api.xml.XmlNode.ID_COMBINATION_MODE_ATTRIBUTE;
+import static org.apache.maven.api.xml.XmlNode.KEYS_COMBINATION_MODE_ATTRIBUTE;
+import static org.apache.maven.api.xml.XmlNode.SELF_COMBINATION_MODE_ATTRIBUTE;
+import static org.apache.maven.api.xml.XmlNode.SELF_COMBINATION_OVERRIDE;
+import static org.apache.maven.api.xml.XmlNode.SELF_COMBINATION_REMOVE;
 
-    /**
-     * @since 3.2.0
-     * @return input location
-     */
-    public Object getInputLocation() {
-        return location;
-    }
+public class XmlNodeUtil {

Review Comment:
   I prefer the Guava convention of using plurals for utility classes; e.g. 
instead of XmlNodeUtil consider XmlNodes



##########
api/maven-api-xml/src/main/java/org/apache/maven/api/xml/XmlNode.java:
##########
@@ -18,121 +18,244 @@
  */
 package org.apache.maven.api.xml;
 
+import javax.xml.stream.XMLStreamException;
+
+import java.io.Serializable;
+import java.io.StringWriter;
 import java.util.List;
+import java.util.ListIterator;
 import java.util.Map;
-
-import org.apache.maven.api.annotations.Experimental;
-import org.apache.maven.api.annotations.Immutable;
-import org.apache.maven.api.annotations.Nonnull;
-import org.apache.maven.api.annotations.Nullable;
-import org.apache.maven.api.annotations.ThreadSafe;
+import java.util.Objects;
+import java.util.function.Function;
 
 /**
- * An immutable xml node.
- *
- * @since 4.0.0
+ *  NOTE: remove all the util code in here when separated, this class should 
be pure data.
  */
-@Experimental
-@ThreadSafe
-@Immutable
-public interface XmlNode {
+public class XmlNode implements Serializable {
+    private static final long serialVersionUID = 2567894443061173996L;
 
-    String CHILDREN_COMBINATION_MODE_ATTRIBUTE = "combine.children";
+    public static final String CHILDREN_COMBINATION_MODE_ATTRIBUTE = 
"combine.children";
 
-    String CHILDREN_COMBINATION_MERGE = "merge";
+    public static final String CHILDREN_COMBINATION_MERGE = "merge";
 
-    String CHILDREN_COMBINATION_APPEND = "append";
+    public static final String CHILDREN_COMBINATION_APPEND = "append";
 
     /**
      * This default mode for combining children DOMs during merge means that 
where element names match, the process will
      * try to merge the element data, rather than putting the dominant and 
recessive elements (which share the same
      * element name) as siblings in the resulting DOM.
      */
-    String DEFAULT_CHILDREN_COMBINATION_MODE = CHILDREN_COMBINATION_MERGE;
+    public static final String DEFAULT_CHILDREN_COMBINATION_MODE = 
CHILDREN_COMBINATION_MERGE;
 
-    String SELF_COMBINATION_MODE_ATTRIBUTE = "combine.self";
+    public static final String SELF_COMBINATION_MODE_ATTRIBUTE = 
"combine.self";
 
-    String SELF_COMBINATION_OVERRIDE = "override";
+    public static final String SELF_COMBINATION_OVERRIDE = "override";
 
-    String SELF_COMBINATION_MERGE = "merge";
+    public static final String SELF_COMBINATION_MERGE = "merge";
 
-    String SELF_COMBINATION_REMOVE = "remove";
+    public static final String SELF_COMBINATION_REMOVE = "remove";
 
     /**
      * In case of complex XML structures, combining can be done based on id.
      */
-    String ID_COMBINATION_MODE_ATTRIBUTE = "combine.id";
+    public static final String ID_COMBINATION_MODE_ATTRIBUTE = "combine.id";
 
     /**
      * In case of complex XML structures, combining can be done based on keys.
      * This is a comma separated list of attribute names.
      */
-    String KEYS_COMBINATION_MODE_ATTRIBUTE = "combine.keys";
+    public static final String KEYS_COMBINATION_MODE_ATTRIBUTE = 
"combine.keys";
 
     /**
      * This default mode for combining a DOM node during merge means that 
where element names match, the process will
      * try to merge the element attributes and values, rather than overriding 
the recessive element completely with the
      * dominant one. This means that wherever the dominant element doesn't 
provide the value or a particular attribute,
      * that value or attribute will be set from the recessive DOM node.
      */
-    String DEFAULT_SELF_COMBINATION_MODE = SELF_COMBINATION_MERGE;
+    public static final String DEFAULT_SELF_COMBINATION_MODE = 
SELF_COMBINATION_MERGE;
+
+    protected final String prefix;

Review Comment:
   Yes it is. They'd just be in a different class. 



-- 
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: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to