Address review comments

certbot-apache/certbot_apache/interfaces.py

@@ -193,7 +193,9 @@ class CommentNode(ParserNode):
         :param bool dirty: Boolean flag for denoting if this CommentNode has been
             created or changed after the last save. Default: False.
         """
-        super(CommentNode, self).__init__(**kwargs)  # pragma: no cover
+        super(CommentNode, self).__init__(ancestor=kwargs['ancestor'],
+                                          dirty=kwargs.get('dirty', False),
+                                          filepath=kwargs['filepath'])  # pragma: no cover
 
 
 @six.add_metaclass(abc.ABCMeta)
@@ -224,7 +226,6 @@ class DirectiveNode(ParserNode):
 
     """
 
-    # pylint: disable=useless-super-delegation
     @abc.abstractmethod
     def __init__(self, **kwargs):
         """
@@ -245,7 +246,9 @@ class DirectiveNode(ParserNode):
             unmatched conditional configuration block. Default: True.
 
         """
-        super(DirectiveNode, self).__init__(**kwargs)  # pragma: no cover
+        super(DirectiveNode, self).__init__(ancestor=kwargs['ancestor'],
+                                            dirty=kwargs.get('dirty', False),
+                                            filepath=kwargs['filepath'])  # pragma: no cover
 
     @abc.abstractmethod
     def set_parameters(self, parameters):

certbot-apache/certbot_apache/parsernode_util.py

@@ -27,7 +27,10 @@ def validate_kwargs(kwargs, required_names):
 
 def parsernode_kwargs(kwargs):
     """
-    Validates keyword arguments for ParserNode.
+    Validates keyword arguments for ParserNode. This function modifies the kwargs
+    dictionary, and hence the returned dictionary should be used instead in the
+    caller function instead of the original kwargs.
+
 
     :param dict kwargs: Keyword argument dictionary to validate.
 
@@ -41,7 +44,10 @@ def parsernode_kwargs(kwargs):
 def commentnode_kwargs(kwargs):
     """
     Validates keyword arguments for CommentNode and sets the default values for
-    optional kwargs.
+    optional kwargs. This function modifies the kwargs dictionary, and hence the
+    returned dictionary should be used instead in the caller function instead of
+    the original kwargs.
+
 
     :param dict kwargs: Keyword argument dictionary to validate.
 
@@ -54,15 +60,18 @@ def commentnode_kwargs(kwargs):
     return comment, kwargs
 
 
-def node_kwargs(kwargs):
+def directivenode_kwargs(kwargs):
     """
     Validates keyword arguments for DirectiveNode and BlockNode and sets the
-    default values for optional kwargs.
+    default values for optional kwargs. This function modifies the kwargs
+    dictionary, and hence the returned dictionary should be used instead in the
+    caller function instead of the original kwargs.
 
     :param dict kwargs: Keyword argument dictionary to validate.
 
     :returns: Tuple of validated and prepared arguments and ParserNode kwargs.
     """
+
     kwargs.setdefault("dirty", False)
     kwargs.setdefault("enabled", True)
     kwargs.setdefault("parameters", ())

certbot-apache/certbot_apache/tests/parsernode_test.py

@@ -44,7 +44,7 @@ class DummyDirectiveNode(DummyParserNode):
         """
         Initializes the DirectiveNode instance and sets its instance variables.
         """
-        name, parameters, enabled, kwargs = util.node_kwargs(kwargs)
+        name, parameters, enabled, kwargs = util.directivenode_kwargs(kwargs)
         self.name = name
         self.parameters = parameters
         self.enabled = enabled

certbot-apache/certbot_apache/tests/parsernode_util_test.py

@@ -1,32 +1,86 @@
 """ Tests for ParserNode utils """
 import unittest
 
-from certbot_apache.tests import parsernode_test
+from certbot_apache import parsernode_util as util
 
 
 class ParserNodeUtilTest(unittest.TestCase):
     """Tests for ParserNode utils"""
-    def test_unknown_parameter(self):
-        params = {
-            "comment": "x",
+
+    def _setup_parsernode(self):
+        """ Sets up kwargs dict for ParserNode """
+        return {
             "ancestor": None,
             "dirty": False,
             "filepath": "/tmp",
-            "unknown": "x"
         }
-        self.assertRaises(TypeError, parsernode_test.DummyCommentNode, **params)
-        params["name"] = "unnamed"
-        params.pop("comment")
-        self.assertRaises(TypeError, parsernode_test.DummyDirectiveNode, **params)
-        self.assertRaises(TypeError, parsernode_test.DummyBlockNode, **params)
+
+    def _setup_commentnode(self):
+        """ Sets up kwargs dict for CommentNode """
+
+        pn = self._setup_parsernode()
+        pn["comment"] = "x"
+        return pn
+
+    def _setup_directivenode(self):
+        """ Sets up kwargs dict for DirectiveNode """
+
+        pn = self._setup_parsernode()
+        pn["name"] = "Name"
+        pn["parameters"] = ("first",)
+        pn["enabled"] = True
+        return pn
+
+    def test_unknown_parameter(self):
+        params = self._setup_parsernode()
+        params["unknown"] = "unknown"
+        self.assertRaises(TypeError, util.parsernode_kwargs, params)
+
+        params = self._setup_commentnode()
+        params["unknown"] = "unknown"
+        self.assertRaises(TypeError, util.commentnode_kwargs, params)
+
+        params = self._setup_directivenode()
+        params["unknown"] = "unknown"
+        self.assertRaises(TypeError, util.directivenode_kwargs, params)
+
+    def test_parsernode(self):
+        params = self._setup_parsernode()
+        ctrl = self._setup_parsernode()
+
+        ancestor, dirty, filepath = util.parsernode_kwargs(params)
+        self.assertEqual(ancestor, ctrl["ancestor"])
+        self.assertEqual(dirty, ctrl["dirty"])
+        self.assertEqual(filepath, ctrl["filepath"])
+
+    def test_commentnode(self):
+        params = self._setup_commentnode()
+        ctrl = self._setup_commentnode()
+
+        comment, _ = util.commentnode_kwargs(params)
+        self.assertEqual(comment, ctrl["comment"])
+
+    def test_directivenode(self):
+        params = self._setup_directivenode()
+        ctrl = self._setup_directivenode()
+
+        name, parameters, enabled, _ = util.directivenode_kwargs(params)
+        self.assertEqual(name, ctrl["name"])
+        self.assertEqual(parameters, ctrl["parameters"])
+        self.assertEqual(enabled, ctrl["enabled"])
 
     def test_missing_required(self):
-        params = {
-            "ancestor": None,
-            "dirty": False,
-            "filepath": "/tmp",
-        }
-        self.assertRaises(TypeError, parsernode_test.DummyCommentNode, **params)
+        c_params = self._setup_commentnode()
+        c_params.pop("comment")
+        self.assertRaises(TypeError, util.commentnode_kwargs, c_params)
+
+        d_params = self._setup_directivenode()
+        d_params.pop("ancestor")
+        self.assertRaises(TypeError, util.directivenode_kwargs, d_params)
+
+        p_params = self._setup_parsernode()
+        p_params.pop("filepath")
+        self.assertRaises(TypeError, util.parsernode_kwargs, p_params)
 
 
 if __name__ == "__main__":