Only skimmed through the PR. API compatibility holds.

It does change behavior though, there are values that are parsed different after this PR.
It doesn't affect the most common launch files, but it might break some uncommon ones.

It doesn't affect the most common launch files, but it might break some uncommon ones.

Question is, do we consider the old behavior a bug or valid but different? I was leaning towards this is a bugfix (correction to intended behavior), but I don't know.

#443 also needs to be backport (#438 was causing issues in CI).

Question is, do we consider the old behavior a bug or valid but different? I was leaning towards this is a bugfix (correction to intended behavior), but I don't know.

I don't remember exactly, I think that some cases were valid but different (some cases were maybe a bug).
e.g.

<param name="asd" value="''3''"/>

was a parameter with a value '3', using double single quotes was valid and the outer ones were eliminated.
Now that string isn't valid, you have to instead use:

<param name="asd" value="3" type="str"/>

#443 also needs to be backport (#438 was causing issues in CI).

Good catch.

Now that string isn't valid

Good point, and this is indeed breaking behavior @jacobperron. We may have to reconsider.

Having said that, it's also true that quoting was a bit of an obscure corner of launch frontends.
I mean, we knew that:

using double single quotes was valid and the outer ones were eliminated.

but I don't think we ever documented that behavior. Or at least I don't remember seeing it written anywhere.

Having said that, it's also true that quoting was a bit of an obscure corner of launch frontends.

True. I'm ok with backporting this, but it's a potentially breaking change.

This is primarily an attempt to fix ros2/launch_ros#214 (which is a bug in the parser), but it seemed not trivial to fix. I hadn't considered the behavior change. It would be good to know what corner cases this breaks exactly before proceeding. If we decide to merge this, then hopefully we can enumerate the cases this breaks (and show the preferred syntax).

This is primarily an attempt to fix ros2/launch_ros#214 (which is a bug in the parser), but it seemed not trivial to fix. I hadn't considered the behavior change. It would be good to know what corner cases this breaks exactly before proceeding. If we decide to merge this, then hopefully we can enumerate the cases this breaks (and show the preferred syntax).

The only one I remember is the one commented before #530 (comment), I'm not aware of other corner cases but there might be.

Added backport of #443 (85f01b9).

The only one I remember is the one commented before #530 (comment), I'm not aware of other corner cases but there might be.

<param name="asd" value="''3''"/>

IMO, this was a workaround because of a bug. I think it's worth fixing this with a release note that folks no longer need to do this extra quoting. If everyone agrees with this, I can add a note to the Foxy release page (for the upcoming patch release).

CI together with ros2/launch_ros#265:

• Linux
• Linux-aarch64
• macOS
• Windows

Edit: Test failures are unrelated to this change.

I've added a release note for the next patch release: ros2/ros2_documentation#2074

👨‍🌾 I think this backport caused a test regression in all the foxy jobs.
Reference: https://build.ros2.org/view/Fci/job/Fci__nightly-release_ubuntu_focal_amd64/335/

This is the test is failing: https://github.com/ros2/launch_ros/blob/c9b6b7057d19b3cba67072818d499370cb14f163/test_launch_ros/test/test_launch_ros/frontend/test_node_frontend.py#L167
Should the test be updated to match the PR or is this an unexpected side effect?

PTAL @jacobperron

@Blast545 My mistake, I forgot to merge the connected PR ros2/launch_ros#265, which I think should resolve the issue.

No prob, thanks for taking a look!