Address further PR feedback

Address ConfigNode PR feedback, including
  * Add @Override tags for all tokens() methods
  * Make `children()` method in ConfigNodeComplexValue final
  * Rename ConfigNodeKey to ConfigNodePath
  * Rename ConfigNodeKeyValue to ConfigNodeField
  * Modify PathParser so it can parse a string into either a Path
    or a ConfigNodePath

config/src/main/java/com/typesafe/config/impl/ConfigNodeComplexValue.java

@@ -3,8 +3,6 @@
  */
 package com.typesafe.config.impl;
 
-import com.typesafe.config.ConfigNode;
-
 import java.util.*;
 
 final class ConfigNodeComplexValue extends AbstractConfigNodeValue {
@@ -14,10 +12,11 @@ final class ConfigNodeComplexValue extends AbstractConfigNodeValue {
         this.children = new ArrayList(children);
     }
 
-    public Iterable<AbstractConfigNode> children() {
+    final public Iterable<AbstractConfigNode> children() {
         return children;
     }
 
+    @Override
     protected Collection<Token> tokens() {
         ArrayList<Token> tokens = new ArrayList();
         for (AbstractConfigNode child : children) {
@@ -28,62 +27,61 @@ final class ConfigNodeComplexValue extends AbstractConfigNodeValue {
 
     protected ConfigNodeComplexValue changeValueOnPath(Path desiredPath, AbstractConfigNodeValue value) {
         ArrayList<AbstractConfigNode> childrenCopy = new ArrayList(children);
-        boolean replaced = value == null;
-        ConfigNodeKeyValue node;
-        Path key;
+        // Copy the value so we can change it to null but not modify the original parameter
+        AbstractConfigNodeValue valueCopy = value;
         for (int i = children.size() - 1; i >= 0; i--) {
-            if (!(children.get(i) instanceof ConfigNodeKeyValue)) {
+            if (!(children.get(i) instanceof ConfigNodeField)) {
                 continue;
             }
-            node = (ConfigNodeKeyValue)children.get(i);
-            key = node.key().value();
+            ConfigNodeField node = (ConfigNodeField)children.get(i);
+            Path key = node.path().value();
             if (key.equals(desiredPath)) {
-                if (!replaced) {
-                    childrenCopy.set(i, node.replaceValue(value));
-                    replaced = true;
-                }
-                else
+                if (valueCopy == null)
                     childrenCopy.remove(i);
+                else {
+                    childrenCopy.set(i, node.replaceValue(value));
+                    valueCopy = null;
+                }
             } else if (desiredPath.startsWith(key)) {
                 if (node.value() instanceof ConfigNodeComplexValue) {
                     Path remainingPath = desiredPath.subPath(key.length());
-                    if (!replaced) {
-                        node = node.replaceValue(((ConfigNodeComplexValue) node.value()).changeValueOnPath(remainingPath, value));
-                        if (node.render() != children.get(i).render())
-                            replaced = true;
-                        childrenCopy.set(i, node);
-                    } else {
-                        node = node.replaceValue(((ConfigNodeComplexValue) node.value()).removeValueOnPath(remainingPath));
-                        childrenCopy.set(i, node);
-                    }
+                    childrenCopy.set(i, node.replaceValue(((ConfigNodeComplexValue) node.value()).changeValueOnPath(remainingPath, valueCopy)));
+                    if (valueCopy != null && node.render() != children.get(i).render())
+                        valueCopy = null;
                 }
             }
         }
         return new ConfigNodeComplexValue(childrenCopy);
     }
 
-    public ConfigNodeComplexValue setValueOnPath(Path desiredPath, AbstractConfigNodeValue value) {
-        ConfigNodeComplexValue node = changeValueOnPath(desiredPath, value);
+    public ConfigNodeComplexValue setValueOnPath(String desiredPath, AbstractConfigNodeValue value) {
+        ConfigNodePath path = PathParser.parsePathNode(desiredPath);
+        return setValueOnPath(path, value);
+    }
+
+    private ConfigNodeComplexValue setValueOnPath(ConfigNodePath desiredPath, AbstractConfigNodeValue value) {
+        ConfigNodeComplexValue node = changeValueOnPath(desiredPath.value(), value);
 
         // If the desired Path did not exist, add it
         if (node.render().equals(render())) {
             ArrayList<AbstractConfigNode> childrenCopy = new ArrayList<AbstractConfigNode>(children);
             ArrayList<AbstractConfigNode> newNodes = new ArrayList();
             newNodes.add(new ConfigNodeSingleToken(Tokens.newLine(null)));
-            newNodes.add(new ConfigNodeKey(desiredPath));
+            newNodes.add(desiredPath);
             newNodes.add(new ConfigNodeSingleToken(Tokens.newIgnoredWhitespace(null, " ")));
             newNodes.add(new ConfigNodeSingleToken(Tokens.COLON));
             newNodes.add(new ConfigNodeSingleToken(Tokens.newIgnoredWhitespace(null, " ")));
             newNodes.add(value);
             newNodes.add(new ConfigNodeSingleToken(Tokens.newLine(null)));
-            childrenCopy.add(new ConfigNodeKeyValue(newNodes));
+            childrenCopy.add(new ConfigNodeField(newNodes));
             node = new ConfigNodeComplexValue(childrenCopy);
         }
         return node;
     }
 
-    public ConfigNodeComplexValue removeValueOnPath(Path desiredPath) {
-        return changeValueOnPath(desiredPath, null);
+    public ConfigNodeComplexValue removeValueOnPath(String desiredPath) {
+        Path path = PathParser.parsePath(desiredPath);
+        return changeValueOnPath(path, null);
     }
 
 }

config/src/main/java/com/typesafe/config/impl/ConfigNodeField.java

@@ -4,18 +4,18 @@
 package com.typesafe.config.impl;
 
 import com.typesafe.config.ConfigException;
-import com.typesafe.config.ConfigNode;
 
 import java.util.ArrayList;
 import java.util.Collection;
 
-final class ConfigNodeKeyValue extends AbstractConfigNode {
+final class ConfigNodeField extends AbstractConfigNode {
     final private ArrayList<AbstractConfigNode> children;
 
-    public ConfigNodeKeyValue(Collection<AbstractConfigNode> children) {
+    public ConfigNodeField(Collection<AbstractConfigNode> children) {
         this.children = new ArrayList(children);
     }
 
+    @Override
     protected Collection<Token> tokens() {
         ArrayList<Token> tokens = new ArrayList();
         for (AbstractConfigNode child : children) {
@@ -24,15 +24,15 @@ final class ConfigNodeKeyValue extends AbstractConfigNode {
         return tokens;
     }
 
-    public ConfigNodeKeyValue replaceValue(AbstractConfigNodeValue newValue) {
+    public ConfigNodeField replaceValue(AbstractConfigNodeValue newValue) {
         ArrayList<AbstractConfigNode> childrenCopy = new ArrayList(children);
         for (int i = 0; i < childrenCopy.size(); i++) {
             if (childrenCopy.get(i) instanceof AbstractConfigNodeValue) {
                 childrenCopy.set(i, newValue);
-                return new ConfigNodeKeyValue(childrenCopy);
+                return new ConfigNodeField(childrenCopy);
             }
         }
-        throw new ConfigException.BugOrBroken("KeyValue node doesn't have a value");
+        throw new ConfigException.BugOrBroken("Field node doesn't have a value");
     }
 
     public AbstractConfigNodeValue value() {
@@ -41,15 +41,15 @@ final class ConfigNodeKeyValue extends AbstractConfigNode {
                 return (AbstractConfigNodeValue)children.get(i);
             }
         }
-        throw new ConfigException.BugOrBroken("KeyValue node doesn't have a value");
+        throw new ConfigException.BugOrBroken("Field node doesn't have a value");
     }
 
-    public ConfigNodeKey key() {
+    public ConfigNodePath path() {
         for (int i = 0; i < children.size(); i++) {
-            if (children.get(i) instanceof ConfigNodeKey) {
-                return (ConfigNodeKey)children.get(i);
+            if (children.get(i) instanceof ConfigNodePath) {
+                return (ConfigNodePath)children.get(i);
             }
         }
-        throw new ConfigException.BugOrBroken("KeyValue node doesn't have a key");
+        throw new ConfigException.BugOrBroken("Field node doesn't have a path");
     }
 }

config/src/main/java/com/typesafe/config/impl/ConfigNodeKey.java

@@ -1,21 +0,0 @@
-/**
- *   Copyright (C) 2015 Typesafe Inc. <http://typesafe.com>
- */
-package com.typesafe.config.impl;
-
-import java.util.Collection;
-
-final class ConfigNodeKey extends AbstractConfigNode {
-    final private Path key;
-    ConfigNodeKey(Path key) {
-        this.key = key;
-    }
-
-    protected Collection<Token> tokens() {
-        return key.tokens();
-    }
-
-    protected Path value() {
-        return key;
-    }
-}

config/src/main/java/com/typesafe/config/impl/ConfigNodePath.java

@@ -0,0 +1,25 @@
+/**
+ *   Copyright (C) 2015 Typesafe Inc. <http://typesafe.com>
+ */
+package com.typesafe.config.impl;
+
+import java.util.ArrayList;
+import java.util.Collection;
+
+final class ConfigNodePath extends AbstractConfigNode {
+    final private Path path;
+    final ArrayList<Token> tokens;
+    ConfigNodePath(Path path, Collection<Token> tokens) {
+        this.path = path;
+        this.tokens = new ArrayList<Token>(tokens);
+    }
+
+    @Override
+    protected Collection<Token> tokens() {
+        return tokens;
+    }
+
+    protected Path value() {
+        return path;
+    }
+}

config/src/main/java/com/typesafe/config/impl/ConfigNodeSimpleValue.java

@@ -12,6 +12,7 @@ class ConfigNodeSimpleValue extends AbstractConfigNodeValue {
         token = value;
     }
 
+    @Override
     protected Collection<Token> tokens() {
         return Collections.singletonList(token);
     }

config/src/main/java/com/typesafe/config/impl/ConfigNodeSingleToken.java

@@ -12,6 +12,7 @@ final class ConfigNodeSingleToken extends AbstractConfigNode{
         token = t;
     }
 
+    @Override
     protected Collection<Token> tokens() {
         return Collections.singletonList(token);
     }

config/src/main/java/com/typesafe/config/impl/Path.java

@@ -15,22 +15,12 @@ final class Path {
     final private String first;
     final private Path remainder;
 
-    // We only need to keep track of this for top-level paths created with
-    // parsePath, so this will be empty or null for all other cases
-    final private ArrayList<Token> tokens;
-
     Path(String first, Path remainder) {
-        this(first, remainder, new ArrayList<Token>());
-    }
-
-    Path(String first, Path remainder, Collection<Token> tokens) {
         this.first = first;
         this.remainder = remainder;
-        this.tokens = new ArrayList<Token>(tokens);
     }
 
     Path(String... elements) {
-        this.tokens = null;
         if (elements.length == 0)
             throw new ConfigException.BugOrBroken("empty path");
         this.first = elements[0];
@@ -52,7 +42,6 @@ final class Path {
 
     // append all the paths in the iterator together into one path
     Path(Iterator<Path> i) {
-        this.tokens = null;
         if (!i.hasNext())
             throw new ConfigException.BugOrBroken("empty path");
 
@@ -217,10 +206,6 @@ final class Path {
         }
     }
 
-    protected Collection<Token> tokens() {
-        return tokens;
-    }
-
     @Override
     public String toString() {
         StringBuilder sb = new StringBuilder();

config/src/main/java/com/typesafe/config/impl/PathBuilder.java

@@ -14,17 +14,8 @@ final class PathBuilder {
     final private Stack<String> keys;
     private Path result;
 
-    // the tokens only matter for top-level paths created with parsePath, in all
-    // other cases this will be empty
-    final private ArrayList<Token> tokens;
-
     PathBuilder() {
-        this(new ArrayList<Token>());
-    }
-
-    PathBuilder(Collection<Token> tokens) {
         keys = new Stack<String>();
-        this.tokens = new ArrayList<Token>(tokens);
     }
 
     private void checkCanAppend() {
@@ -62,7 +53,7 @@ final class PathBuilder {
             Path remainder = null;
             while (!keys.isEmpty()) {
                 String key = keys.pop();
-                remainder = new Path(key, remainder, tokens);
+                remainder = new Path(key, remainder);
             }
             result = remainder;
         }

config/src/main/java/com/typesafe/config/impl/PathParser.java

@@ -32,6 +32,19 @@ final class PathParser {
 
     static ConfigOrigin apiOrigin = SimpleConfigOrigin.newSimple("path parameter");
 
+    static ConfigNodePath parsePathNode(String path) {
+        StringReader reader = new StringReader(path);
+
+        try {
+            Iterator<Token> tokens = Tokenizer.tokenize(apiOrigin, reader,
+                    ConfigSyntax.CONF);
+            tokens.next(); // drop START
+            return parsePathNodeExpression(tokens, apiOrigin, path);
+        } finally {
+            reader.close();
+        }
+    }
+
     static Path parsePath(String path) {
         Path speculated = speculativeFastParsePath(path);
         if (speculated != null)
@@ -51,14 +64,31 @@ final class PathParser {
 
     protected static Path parsePathExpression(Iterator<Token> expression,
                                             ConfigOrigin origin) {
-        return parsePathExpression(expression, origin, null);
+        return parsePathExpression(expression, origin, null, null);
+    }
+
+    protected static Path parsePathExpression(Iterator<Token> expression,
+                                              ConfigOrigin origin, String originalText) {
+        return parsePathExpression(expression, origin, originalText, null);
+    }
+
+    protected static ConfigNodePath parsePathNodeExpression(Iterator<Token> expression,
+                                                            ConfigOrigin origin) {
+        return parsePathNodeExpression(expression, origin, null);
+    }
+
+    protected static ConfigNodePath parsePathNodeExpression(Iterator<Token> expression,
+                                                            ConfigOrigin origin, String originalText) {
+        ArrayList<Token> pathTokens = new ArrayList<Token>();
+        Path path = parsePathExpression(expression, origin, originalText, pathTokens);
+        return new ConfigNodePath(path, pathTokens);
     }
 
     // originalText may be null if not available
     protected static Path parsePathExpression(Iterator<Token> expression,
-                                            ConfigOrigin origin, String originalText) {
+                                            ConfigOrigin origin, String originalText,
+                                            ArrayList<Token> pathTokens) {
         // each builder in "buf" is an element in the path.
-        ArrayList<Token> pathTokens = new ArrayList<Token>();
         List<Element> buf = new ArrayList<Element>();
         buf.add(new Element("", false));
 
@@ -69,7 +99,9 @@ final class PathParser {
 
         while (expression.hasNext()) {
             Token t = expression.next();
-            pathTokens.add(t);
+
+            if (pathTokens != null)
+                pathTokens.add(t);
 
             // Ignore all IgnoredWhitespace tokens
             if (Tokens.isIgnoredWhitespace(t))
@@ -116,7 +148,7 @@ final class PathParser {
             }
         }
 
-        PathBuilder pb = new PathBuilder(pathTokens);
+        PathBuilder pb = new PathBuilder();
         for (Element e : buf) {
             if (e.sb.length() == 0 && !e.canBeEmpty) {
                 throw new ConfigException.BadPath(
@@ -194,7 +226,7 @@ final class PathParser {
         ArrayList<Token> tokens = new ArrayList<Token>();
         tokens.add(Tokens.newUnquotedText(null, s));
         // this works even if splitAt is -1; then we start the substring at 0
-        Path withOneMoreElement = new Path(s.substring(splitAt + 1, end), tail, tokens);
+        Path withOneMoreElement = new Path(s.substring(splitAt + 1, end), tail);
         if (splitAt < 0) {
             return withOneMoreElement;
         } else {

config/src/test/scala/com/typesafe/config/impl/ConfigNodeTest.scala

@@ -21,10 +21,10 @@ class ConfigNodeTest extends TestUtils {
         assertEquals(node.render(), token.tokenText())
     }
 
-    private def keyValueNodeTest(key: ConfigNodeKey, value: AbstractConfigNodeValue, trailingWhitespace: ConfigNodeSingleToken, newValue: AbstractConfigNodeValue) {
+    private def keyValueNodeTest(key: ConfigNodePath, value: AbstractConfigNodeValue, trailingWhitespace: ConfigNodeSingleToken, newValue: AbstractConfigNodeValue) {
         val keyValNode = nodeKeyValuePair(key, value, trailingWhitespace)
         assertEquals(key.render() + " : " + value.render() + trailingWhitespace.render(), keyValNode.render())
-        assertEquals(key.render, keyValNode.key().render())
+        assertEquals(key.render, keyValNode.path().render())
         assertEquals(value.render, keyValNode.value().render())
 
         val newKeyValNode = keyValNode.replaceValue(newValue)
@@ -37,7 +37,7 @@ class ConfigNodeTest extends TestUtils {
                                        nodeKeyValuePair(nodeWhitespace("       "), configNodeKey(key),value, nodeWhitespace("    ")),
                                        nodeCloseBrace)
         val complexNode = configNodeComplexValue(complexNodeChildren)
-        val newNode = complexNode.setValueOnPath(Path.newPath(key), newValue)
+        val newNode = complexNode.setValueOnPath(key, newValue)
         val origText = "{       " + key + " : " + value.render() + "    }"
         val finalText = "{       " + key + " : " + newValue.render() + "    }"
 
@@ -55,13 +55,13 @@ class ConfigNodeTest extends TestUtils {
         val finalText = key.render() + " : 15"
 
         assertEquals(origText, complexNode.render())
-        assertEquals(finalText, complexNode.setValueOnPath(Path.newPath("foo"), nodeInt(15)).render())
+        assertEquals(finalText, complexNode.setValueOnPath("foo", nodeInt(15)).render())
     }
 
     private def nonExistentPathTest(value: AbstractConfigNodeValue) {
         val node = configNodeComplexValue(List(nodeKeyValuePair(configNodeKey("bar"), nodeInt(15))))
         assertEquals("bar : 15", node.render())
-        val newNode = node.setValueOnPath(Path.newPath("foo"), value)
+        val newNode = node.setValueOnPath("foo", value)
         val finalText = "bar : 15\nfoo : " + value.render() + "\n"
         assertEquals(finalText, newNode.render())
     }
@@ -199,14 +199,14 @@ class ConfigNodeTest extends TestUtils {
 
         //Can replace settings in nested maps
         // Paths with quotes in the name are treated as a single Path, rather than multiple sub-paths
-        var newNode = origNode.setValueOnPath(Path.newPath("baz.\"abc.def\""), configNodeSimpleValue(tokenTrue))
-        newNode = newNode.setValueOnPath(Path.newPath("baz.abc.def"), configNodeSimpleValue(tokenFalse))
+        var newNode = origNode.setValueOnPath("baz.\"abc.def\"", configNodeSimpleValue(tokenTrue))
+        newNode = newNode.setValueOnPath("baz.abc.def", configNodeSimpleValue(tokenFalse))
 
         // Repeats are removed from nested maps
-        newNode = newNode.setValueOnPath(Path.newPath("baz.abc.ghi"), configNodeSimpleValue(tokenUnquoted("randomunquotedString")))
+        newNode = newNode.setValueOnPath("baz.abc.ghi", configNodeSimpleValue(tokenUnquoted("randomunquotedString")))
 
         // Missing paths are added to the top level if they don't appear anywhere, including in nested maps
-        newNode = newNode.setValueOnPath(Path.newPath("baz.abc.\"this.does.not.exist@@@+$#\".end"), configNodeSimpleValue(tokenUnquoted("doesnotexist")))
+        newNode = newNode.setValueOnPath("baz.abc.\"this.does.not.exist@@@+$#\".end", configNodeSimpleValue(tokenUnquoted("doesnotexist")))
 
         // The above operations cause the resultant map to be rendered properly
         assertEquals(finalText, newNode.render())

config/src/test/scala/com/typesafe/config/impl/TestUtils.scala

@@ -667,10 +667,7 @@ abstract trait TestUtils {
         new ConfigNodeSimpleValue(value)
     }
 
-    def configNodeKey(path: String) = {
-        val parsedPath = PathParser.parsePath(path)
-        new ConfigNodeKey(parsedPath)
-    }
+    def configNodeKey(path: String) = PathParser.parsePathNode(path)
 
     def configNodeBasic(value: Token) = {
         new ConfigNodeSingleToken(value: Token)
@@ -689,17 +686,17 @@ abstract trait TestUtils {
     def nodeComma = new ConfigNodeSingleToken(Tokens.COMMA)
     def nodeLine(line: Integer) = new ConfigNodeSingleToken(tokenLine(line))
     def nodeWhitespace(whitespace: String) = new ConfigNodeSingleToken(tokenWhitespace(whitespace))
-    def nodeKeyValuePair(key: ConfigNodeKey, value: AbstractConfigNodeValue) = {
+    def nodeKeyValuePair(key: ConfigNodePath, value: AbstractConfigNodeValue) = {
         val nodes = List(key, nodeSpace, nodeColon, nodeSpace, value)
-        new ConfigNodeKeyValue(nodes.asJavaCollection)
+        new ConfigNodeField(nodes.asJavaCollection)
     }
-    def nodeKeyValuePair(key: ConfigNodeKey, value: AbstractConfigNodeValue, trailingWhitespace: ConfigNodeSingleToken) = {
+    def nodeKeyValuePair(key: ConfigNodePath, value: AbstractConfigNodeValue, trailingWhitespace: ConfigNodeSingleToken) = {
         val nodes = List(key, nodeSpace, nodeColon, nodeSpace, value, trailingWhitespace)
-        new ConfigNodeKeyValue(nodes.asJavaCollection)
+        new ConfigNodeField(nodes.asJavaCollection)
     }
-    def nodeKeyValuePair(leadingWhitespace: ConfigNodeSingleToken, key: ConfigNodeKey, value: AbstractConfigNodeValue, trailingWhitespace: ConfigNodeSingleToken) = {
+    def nodeKeyValuePair(leadingWhitespace: ConfigNodeSingleToken, key: ConfigNodePath, value: AbstractConfigNodeValue, trailingWhitespace: ConfigNodeSingleToken) = {
         val nodes = List(leadingWhitespace, key, nodeSpace, nodeColon, nodeSpace, value, trailingWhitespace)
-        new ConfigNodeKeyValue(nodes.asJavaCollection)
+        new ConfigNodeField(nodes.asJavaCollection)
     }
     def nodeInt(value: Integer) = new ConfigNodeSimpleValue(tokenInt(value))
     def nodeString(value: String) = new ConfigNodeSimpleValue(tokenString(value))