Removes erroneous unmatched closing parenthesis

[spiderkeys]

Addresses: #124

[clalancette]

This fix is clearly correct, so thanks for that.

It's clear that we aren't testing this functionality at all. Can you please add a test for this? I'll suggest adding a new message in the msg subdirectory here, and then adding in a new test into test/test_interfaces.py.

[spiderkeys]

Will do 👍

[spiderkeys]

@clalancette I've added some types (both .idl and .msg) that include a comprehensive set of built-in types, arrays, and sequences, as well as some basic test cases.

In doing so, I came across one additional error for char array types. See: e925bb3

Let me know if this is what you had in mind. Happy to make any changes.

[clalancette]

This was exactly the kind of thing I had in mind, so thanks for that.

However, I'm now actually a bit confused on what is going on here. Take your new BuiltinTypesMsg.msg message. That tests all of the basic types and the array versions of the basic types. But shouldn't that already have been done via the BasicTypes and Array message in test_interfaces.py? That is, it seems like a lot of this is duplicating tests that are already there. But clearly that isn't the case since this bug exists. Do you have any thoughts on what is going on here?

[sloretz]

That is, it seems like a lot of this is duplicating tests that are already there. But clearly that isn't the case since this bug exists. Do you have any thoughts on what is going on here?

@clalancette I think the difference is this adds a test for char[2] char_array to a message, which isn't currently covererd by test_interface_files. The rest of the fields added in tests in this PR may or may not be duplicating existing tests.

https://github.com/ros2/rosidl_python/pull/125/files#diff-4c54c46da9bed05c8c28519d8a90efacd5ac1bb2590c04b13aba8d893cfd7237R18-R33

[spiderkeys]

Sorry, I kind of forgot about this PR.

As both @sloretz and @clalancette mention, there are likely a number of duplicate test cases - when I originally put this together, my thought process was to very explicitly capture most if not all basic permutations of the builtin types defined by the IDL spec. The proper way to remove the duplication is likely to move my Builtin* type files or a subset of their contents over to the test_interface_files repo so they all live in one place.

https://github.com/ros2/test_interface_files

I don't have much time in the next week or two to tackle this, but if you agree with the above change and someone could make the appropriate changes in test_interface_files, I could cut that aspect out of this PR so the fix can make its way in.

[clalancette]

@clalancette I think the difference is this adds a test for char[2] char_array to a message, which isn't currently covererd by test_interface_files. The rest of the fields added in tests in this PR may or may not be duplicating existing tests.

@sloretz Ah, thanks so much for pointing that out. That was exactly what I was missing.

As both @sloretz and @clalancette mention, there are likely a number of duplicate test cases - when I originally put this together, my thought process was to very explicitly capture most if not all basic permutations of the builtin types defined by the IDL spec.

@spiderkeys Yeah, understood. I think I got bogged down in adding so many test cases, and they seemed like duplicated. What I've done here now is to rebase this onto the latest, and then removed all of the test cases except for the one failure in b5ce601. With that, I think this is easier to review and understand. I'm happy with this now, so I'm going to go ahead and approve, but I'd appreciate a look from you just to make sure you agree with the direction here.

[clalancette]

@spiderkeys Oh, also if you could go in here and rebase to add Signed-off-by tags to every commit, that will make the DCO bot happy. Thanks.

[spiderkeys]

@clalancette I think I've just performed the rebase correctly to amend with signoffs, but let me know if something doesn't look right.

[clalancette]

@clalancette I think I've just performed the rebase correctly to amend with signoffs, but let me know if something doesn't look right.

The signoffs look good, thanks.

I ended up rebasing it again to have it based off of master. With that, this looks good to me so I'll run CI on it next.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Windows

[clalancette]

All of the failures on Windows are known (and most of them should actually be fixed by now). So I'm going to go ahead and merge this; thanks again for the contribution!

[flynneva]

@clalancette following up on this topic - has this bug-fix been backported to other ros2 distros as well? Would you be willing to take PR's to backport it? Would be great to get this fixed in all distros rather than just rolling...especially with the transition to jammy coming up soon I think.

[clalancette]

@clalancette following up on this topic - has this bug-fix been backported to other ros2 distros as well? Would you be willing to take PR's to backport it? Would be great to get this fixed in all distros rather than just rolling...especially with the transition to jammy coming up soon I think.

The transition to Jammy (and Humble) is done now. In terms of this being backported, no, we haven't done it yet. I'll kick it off to see if the bot can do it.

[clalancette]

@Mergifyio backport foxy galactic

[mergify]

backport foxy galactic

✅ Backports have been created

• #158 Removes erroneous unmatched closing parenthesis (backport #125) has been created for branch foxy
• #159 Removes erroneous unmatched closing parenthesis (backport #125) has been created for branch galactic