Allow expressions in comments

[rhaschke]

Implements #299.

[gavanderhoorn]

What would be the escape sequence? As in: how would someone prevent xacro from always trying to evaluate expressions in comments? I have a few comments which contain expressions which should stay expressions (as part of the documentation).

[rhaschke]

Same as in normal text expressions: multiple dollar signs:

xacro/test/test_xacro.py

Lines 573 to 576 in 6d67741

	def test_escaping_dollar_braces(self):
	src = '''<a b="$${foo}" c="$$${foo}" d="text $${foo}" e="text $$${foo}" f="$$(pwd)" />'''
	res = '''<a b="${foo}" c="$${foo}" d="text ${foo}" e="text $${foo}" f="$(pwd)" />'''
	self.assert_matches(self.quick_xacro(src), res)

[gavanderhoorn]

hm, that's a bit annoying, certainly for documentation.

We would then have to add something like:

(the real expression does not use double-dollar signs (ie: $$), this is just for escaping the expression)

otherwise I guarantee some of "my" users will start trying to use $$ in front of variables.

[rhaschke]

I get your point. What syntax do you propose then?

[rhaschke]

I think the key question is where do you want to document what. You still can use single-dollar expressions as documentation in the xacro file. However, they will get replaced in the final file. Does that matter? The documentation was intended for the xacro in this case. If you want to document in the final file, just provide double dollars in the xacro.
Obviously, you cannot target documentation to both files using the very same syntax.

[gavanderhoorn]

The documentation was intended for the xacro in this case.

Yes, that's true.

it will be confusing for users looking at the results of xacro processing to see comments which apparently magically changed from the xacro, but in principle the documentation is for the "xacro side".

---

Edit: so I withdraw the question / remark.

[rhaschke]

see comments which apparently magically changed from the xacro

That's the basic idea of the macro processor 😉
Probably you know that comments in front of <xacro> tags are automatically discarded. In that case, you wouldn't face the problem in the generated doc anymore.

[rhaschke]

I withdraw the question / remark.

So, you approve?

[gavanderhoorn]

I don't really have an opinion on this one.

But that's mostly because there is no use-case described for this.

[rhaschke]

Oh, my use case is to generate comments within a macro, documenting the generated doc:

<robot>
  <xacro:macro name="link" params="name">
    <!-- link ${name} -->
    <link name="${name}"/>
  </xacro:macro>
  <xacro:link name="foo"/>
</robot>

becomes

<robot>
  <!-- link foo -->
  <link name="foo"/>
</robot>

[gavanderhoorn]

Ok.

I would have always expected the generated .urdf (or whatever is the end result) to be consumed by a machine instead of a human, but if humans are supposed to be the ones understanding the output, then having comments in there might make sense.

Is this in the context of the updates to the panda MoveIt config?

[rhaschke]

Yes, indeed, I would like to use the new feature there.

[seowalex]

Just encountered an error because of this, which took awhile to figure out because I didn't expect it to be a problem in an upstream ROS package. Personally, I don't think evaluating anything in comments makes sense, given that no other language does it. While I understand the desire to evalutate expressions to generate better looking documentation, perhaps the default behaviour should be no evaluation, and you can use some sort of escape sequence to force xacro to evaluate them.

[rhaschke]

xacro is an XML pre-processor. Thus, it should evaluate expressions in comments as well, because comments are a part of XML. To facilitate transition I will change the error to a warning only.

[rhaschke]

Releases triggered:

• xacro: 1.13.16-1 in 'melodic/distribution.yaml' [bloom] rosdistro#31975
• xacro: 1.14.12-1 in 'noetic/distribution.yaml' [bloom] rosdistro#31976

Consider using the debian packages from the ROS testing repo until the releases are synced.

[mintar]

xacro is an XML pre-processor. Thus, it should evaluate expressions in comments as well, because comments are a part of XML. To facilitate transition I will change the error to a warning only.

That's certainly a valid point of view.

I'd still prefer xacro wouldn't parse comments by default. As a programmer, I don't expect the preprocessor to break because of some stuff that I have commented out. Right now, the changed behavior of xacro breaks lots of existing .xacro files everywhere that have commented-out sections with substitution args.

[gavanderhoorn]

@rhaschke: would it perhaps be an option to make this an opt-in?

For your use-case, you could enable it. For others, it would be disabled by default.

[rhaschke]

The latest melodic-devel / noetic-devel branch only throw a warning (but don't error) anymore on invalid expressions in the comments.
Doesn't that satisfy you, @mintar?

[peci1]

I second @mintar's request for hiding this behavior behind a non-default parameter. Yes, warnings will not break stuff, but it's still a bit unexpected for me to comment-out a part of code and receive warnings relating to it.

[mintar]

The latest melodic-devel / noetic-devel branch only throw a warning (but don't error) anymore on invalid expressions in the comments.
Doesn't that satisfy you, @mintar?

Not fully. I try to fix all warnings, so if it remains a warning I'll update my .xacro files to remove them - probably by mutilating the code so that xacro doesn't recognize the substitution arg any more, such as inserting a space between $ and {. That will cause problems of course when it's commented back in (and will not even throw a warning/error then, because xacro doesn't recognize that $ {arg nonexisting} is something that it's supposed to handle).

So I'm voting for hiding the new behavior behind a non-default parameter. But ultimately it's your decision.

[rhaschke]

I agree: disabling some broken code by commenting, is a common use case that should be supported.
I'm thinking of these opt-in options:

• a marker indicating that the next comment should be evaluated, e.g.
  <!-- xacro:eval-comment -->[whitespace]<!-- some comment with an ${expression} -->
• or having this marker within the current comment (How to handle the marker and surrounding whitespace, in this case?):
  <!-- xacro:eval-comment some comment with an ${expression} -->
• an additional global cmdline switch to enable comment evaluation

What do you think?

[mintar]

All 3 options are fine with me. Personally, I think options 2 is the best one, because it is most explicit, most robust and also simplest. For example, you have a xacro with comments that should be evaluated, but you also include xacros from other packages. Unaware of your use case, the downstream package maintainer changes their xacro. With option 3, this could break your package, but it's no problem for option 2.

[rhaschke]

Option 3 is most dangerous, no discussion.
For me, option 2 is problematic, because it's not clear how to remove the marker (obviously, you don't want to keep the marker).
With option 1, I would simply drop the comment with the marker and mark the next comment to be kept. If the next XML node isn't actually a comment, the marker flag will be reset to disable comment evaluation. The only exception to this rule would be whitespace text.

[peci1]

What about modified nr. 1:

<!-- xacro:eval-comments -->
  <!-- some comment with an ${expression} -->
  <!-- another comment with an ${expression} -->
<!-- /xacro:eval-comments -->

That would allow enabling evaluation even for blocks of multiple comments (these are pretty commonly generated by IDEs when you comment-out a section of XML files).

[rhaschke]

This would be more verbose for typing 😞
Also, I thought we want to be as conservative as possible. What if you forget to finish disabling comment evaluation?

[peci1]

Okay, then option 2 seems the most reasonable. Your doubts were about implementation, i.e. whether you are able to modify comments from within xacro? If yes, then just removing the verbatim string that enables the processing would be enough, wouldn't it?

[rhaschke]

Your doubts were about implementation, i.e. whether you are able to modify comments from within xacro?
Just removing the verbatim string that enables the processing would be enough, wouldn't it?

No, I wouldn't like to keep all of the surrounding whitespace. But, how much to remove of this whitespace?
Having a separate comment that can be discarded completely, would be much cleaner.

[peci1]

Require exactly 1 witespace at the beginning <!--_ and remove exactly 1 whitespace after the "tag"? That would be crystal clear....

[rhaschke]

What about these variants:

<!--xacro:eval-comments-->
<!-- xacro:eval-comments -->
<!--  xacro:eval-comments:    some text to be kept -->

Removing exactly _xacro:eval-comments seems to be rather restrictive, doesn't it?

[peci1]

I don't really like the colon in the last line of your example. What if someone forgets it? Then the rest of the text would be skipped?

[rhaschke]

I decided to go for a variant of #300 (comment). See #310.