@ivanpauno meta: IMHO, type deduction documentation is design document material.

If you want, I can update ros2/design#208 and ros2/design#207, and create a new PR for yaml design doc.

Eventually, yeah, but let's hold that for a minute. At least until we've settled here.

@hidmic I added this new test case. For having a more uniform behavior, I think this should look like:
'$(env "asd_bsd_qsd_$(test \\\'asd_bds\\\')" \'$(env DEFAULT)_qsd\')'
(this is not a raw a string, I'm escaping both the \ and ')

For that, I would change the following in the grammar:

SINGLE_QUOTED_STRING: (/[^'\\"$]|\$(?=!\()|(?<=\\)\$/ | "\\\"" | "\\'" | "\\")+
SINGLE_QUOTED_RSTRING: (/[^ '\\"$\(\)]|(?<=\\)\(|(?<=\\)\)|\$(?=!\()|(?<=\\)\$/ | "\\\"" | "\\'" | "\\")+

DOUBLE_QUOTED_STRING: (/[^"\\'$]|\$(?=!\()|(?<=\\)\$/ | "\\\"" | "\\'" | "\\")+
DOUBLE_QUOTED_RSTRING: (/[^ "\\'$\(\)]|(?<=\\)\(|(?<=\\)\)|\$(?=!\()|(?<=\\)\$/ | "\\\"" | "\\'" | "\\")+

Basically, I'm stopping allowing any kind of quote inside SINGLE_QUOTED_* and DOUBLE_QUOTED_*, and only allowing it if it is escaped.

Do you agree with that change?

I'm fine with it, though it may be counter intuitive for some accustomed to combine quotes. If we do it, we should clearly document it somewhere. @wjwwood any thoughts on this i.e. on forcing quote escaping everywhere if one wants to keep it raw?

Escaping quotes inside of quotes is ok by me.

@ivanpauno why not replace 'guess' by the absence of a type and make it the default?

Because the type that is specified most of the times is str (substitutions are str).

Same, making str the default.

@ivanpauno hmm, these two bullets are packed with assumptions about downstream implementations and markup languages that I'd prefer to spare. Consider just stating that the types argument states the expected types of a given attribute and whether that results in type coercion or type checking depends on the particular frontend.

Also, now that I think of it, it might make sense to let the user choose between checks and coercions.

@ivanpauno hmm, I'm somewhat inclined to say that these methods don't need nor should be class methods.

parse_description, parse_action should. cls is finally passed to the action parsing methods as parser argument. The parsing methods could call parse.substitution, which can be overridden by the derived class.

Right, my point being that these should be instance methods.

The Parse instances don't have a state now, but it could have it in the future, so I think it is a good idea. I will change parse_action, parse_substitution, parse_description to instance methods.
load, load_parser_extensions and load_parser_implementations will be continue being class methods.
I will stop returning an object of Parse type in the load method (ie: a subclass of Parse) , and start returning an instance of it.
Note: I will also rename load_parser_extensions as load_launch_extensions.

@ivanpauno after going over this file a couple times, I think that using strings for type identification is a bit clunky. We could use type objects as well, which would then simplify the implementation. Thoughts?

The problem is with 'list[int]', etc. Using typing objects doesn't help much in the implementation (i.e.: I don't have an easy way to extract if it was a list, and what is the item type).

Right, but you can check for equality just like you do with strings. And then you don't have to do substring checks.

I still wonder if it wouldn't be better to use typing objects instead of plain strings.

I don't have a strong opinion. Using typing would be cool, but I don't know if it makes it faster or cleaner.

So, between using typing classes and using a stringification of their names, I'm slightly inclined towards the first one. Collection type information is readily available and the API looks cleaner that way IMO. No need to do substring checks anymore.

@ivanpauno meta: we can leave it as is for a first iteration, but I'm not entirely convinced that working with importation side effects is in our best interest in the long run. I wonder if having some sort of index at each package root module that its classes can expose/register themselves into would be cleaner than a having a global one. Maybe @wjwwood can weigh in too on this.

Are you talking about using pkg_resources or something else (sorry I didn't really grok "importation side effects" in this context)?

pkg_resources is known to be a bit slow, but this should be a one time activity. Of course, we don't want launch startup to be slow either, so I don't have a strong feeling one way or the other. We talked about other solutions in catkin tools, and some guy actually commented that they made a replacement system:

catkin/catkin_tools#297

We could use ament index instead, but I don't have any strong feelings about it one way or the other.

It's not so much about performance (though I had not event considered that, thanks for the reference) but the way actions, substitutions and parsers are discovered. It's currently loading a global collection inside launch upon entry point loading.

I was wondering if it wouldn't make sense to move away from that and make exposure somewhat more explicit, and maybe introducing namespaces in the process (which we do not have, we just fail upon tag name collision). Anyways, just a thought. No need to address this now, really.

@ivanpauno meta: I wonder if bool should come before int

No, if not 0 will be converted to bool. If both are possible, it should be an int.
And maybe we should just delete that option. So the rules do not overlap.

If both are possible, it should be an int.

Ok, fair enough, Python coercion will do its job.