Only optionally activate comment evaluation (#310)

According to the discussion in #300, comment evaluation should be an opt-in feature,
primarily because commenting is regularly used to simply disable broken code.
Obviously, in such a use case, comment evaluation would be counterproductive.

Now, comment evaluation can be enabled with a special comment:
`<!-- xacro:eval-comments -->` or `<!-- xacro:eval-comments:on -->`

It remains active for the following comments until:
- the current XML tag's scope is left (or a new tag entered)
- another tag or non-whitespace text is processed
- it becomes explicitly disabled via: `<!-- xacro:eval-comments:off -->`

Reverts 43ceb30, i.e. issues an error on failed expression evaluation again.

src/xacro/__init__.py

@@ -997,7 +997,7 @@ def remove_previous_comments(node):
             return
 
 
-def eval_all(node, macros, symbols, comment_warning_issued=[False]):
+def eval_all(node, macros, symbols):
     """Recursively evaluate node, expanding macros, replacing properties, and evaluating expressions"""
     # evaluate the attributes
     for name, value in node.attributes.items():
@@ -1014,9 +1014,11 @@ def eval_all(node, macros, symbols, comment_warning_issued=[False]):
         pass
 
     node = node.firstChild
+    eval_comments = False
     while node:
         next = node.nextSibling
         if node.nodeType == xml.dom.Node.ELEMENT_NODE:
+            eval_comments = False  # any tag automatically disables comment evaluation
             if node.tagName in ['insert_block', 'xacro:insert_block'] \
                     and check_deprecated_tag(node.tagName):
                 name, = check_attrs(node, ['name'], [])
@@ -1101,19 +1103,17 @@ def eval_all(node, macros, symbols, comment_warning_issued=[False]):
 
         elif node.nodeType == xml.dom.Node.TEXT_NODE:
             node.data = unicode(eval_text(node.data, symbols))
+            if node.data.strip():
+                eval_comments = False # non-empty text disables comment evaluation
+
         elif node.nodeType == xml.dom.Node.COMMENT_NODE:
-            try:
+            if "xacro:eval-comments" in node.data:
+                eval_comments = "xacro:eval-comments:off" not in node.data
+                replace_node(node, by=None) # drop this comment
+            elif eval_comments:
                 node.data = unicode(eval_text(node.data, symbols))
-            except Exception as e:
-                if not comment_warning_issued[0]:
-                    comment_warning_issued[0] = True
-                    msg = unicode(e)
-                    if not msg:
-                        msg = repr(e)
-                    warning("Error resolving an expression in a comment (skipping evaluation):")
-                    warning(msg)
-                    if verbosity > 0:
-                        print_location()
+            else:
+                pass # leave comment as is
 
         node = next
 

test/test_xacro.py

@@ -37,6 +37,7 @@
 
 import ast
 from contextlib import contextmanager
+import itertools
 import math
 import os.path
 import re
@@ -1224,11 +1225,26 @@ def test_xml_namespace_lifting(self):
         self.assert_matches(self.quick_xacro(src), res)
 
     def test_comments(self):
+        original = '<!-- ${name} -->'
+        processed = '<!-- foo -->'
+        enabler = '<!-- xacro:eval-comments{suffix} -->'
+        disabler = enabler.format(suffix=":off")
+
         src = '''<a xmlns:xacro="http://www.ros.org/wiki/xacro">
         <xacro:property name="name" value="foo"/>
-        <!-- ${name} --></a>'''
-        res = '''<a><!-- foo --></a>'''
-        self.assert_matches(self.quick_xacro(src), res)
+        {enabler}{comment}{text}{comment}</a>'''
+        result = '<a>{c1}{text}{c2}</a>'
+        for enable, suffix, text in itertools.product([False, True], ["", ":on", ":off"], ["", " ", " text ", "<tag/>", disabler]):
+          src_params = dict(comment=original, text=text,
+                            enabler=enabler.format(suffix=suffix) if enable else "")
+          enabled = enable and suffix != ":off"
+          res_params = dict(c1=processed if enabled else original, text="" if text == disabler else text,
+                            c2=processed if enabled and not text.strip() and text != disabler else original)
+          try:
+            self.assert_matches(self.quick_xacro(src.format(**src_params)), result.format(**res_params))
+          except AssertionError as e:
+            print("When evaluating\n{}".format(src.format(**src_params)))
+            raise
 
 
 # test class for in-order processing