force lower case for ros package name #12
Conversation
|
Can you please rebase this PR? |
jeising
force-pushed
the
patch-1
branch
3 times, most recently
from
345c192
to
4a0437d
Compare
|
@Karsten1987: Should be fine now |
|
@Karsten1987: Is there anything missing and keeps us from merging this PR? |
|
@michael-poehnl & @Karsten1987: Please review this change :) |
There was a problem hiding this comment.
Thanks @jeising for the patch and please apologize for the late response.
I do request changes mainly for the get_service_description_elements function, as I think that change can be re-written in a more readable way.
Also, I know I am asking a lot here, but I would highly appreciate if you would go the extra mile and actually add unit tests for this logic. I think that stuff should definitely be tested. It's fine if you don't want to do that. I'd open a follow-up ticket for it then.
@michael-poehnl What's the status on this? Is there something new coming to iceoryx? |
|
@Karsten1987: Thanks for your comments! As you can see e.g. in the tests I am not done with the changes yet. I'll ping you once the PR is ready again. I feel "push early" is good to avoid code loss. |
|
@Karsten1987 & @michael-poehnl: Feel free to review the code, thx! |
I'm not sure what @jeising has in mind. There is no type support topic in the near future planned. Frameworks like ROS2 or eCAL solve this on top of iceoryx. So far iceoryx is data agnostic |
Hm .. I thought I read it somewhere |
|
We have to rebase because of #14 Maybe this also affects your test. |
To be able to use `iceoryx`s native applications with `ros 2` based applications the package name defined from service and event name must be valid for `ros 2` as package name. Forcing lowercase in the package name only partially fixes this issue. As far as I know there are changes planned regarding types in `iceoryx`, so I would propose this fix, till we can focus on the new type handling. Signed-off-by: Johannes Jeising <Johannes.Jeising@de.bosch.com>
Signed-off-by: Johannes Jeising <Johannes.Jeising@de.bosch.com>
Signed-off-by: Johannes Jeising <Johannes.Jeising@de.bosch.com>
Signed-off-by: Johannes Jeising <Johannes.Jeising@de.bosch.com>
Signed-off-by: Johannes Jeising <Johannes.Jeising@de.bosch.com>
Signed-off-by: Johannes Jeising <Johannes.Jeising@de.bosch.com>
Signed-off-by: Johannes Jeising <Johannes.Jeising@de.bosch.com>
Signed-off-by: Johannes Jeising <Johannes.Jeising@de.bosch.com>
Signed-off-by: Johannes Jeising <Johannes.Jeising@de.bosch.com>
Signed-off-by: Johannes Jeising <Johannes.Jeising@de.bosch.com>
|
|
||
| TEST(NameConverisonTests, get_service_description_elements_revers) | ||
| { | ||
| auto service_description = rmw_iceoryx_cpp::get_service_description_elements( |
There was a problem hiding this comment.
why does this API have to be public? Can't we just test get_name_n_type_from_iceoryx_service_description and validate the string with std::get<0>?
There was a problem hiding this comment.
I guess you mean get_iceoryx_service_description. The issue is that there is a type support required and I had no idea, where to get it from. I also did not want to start mocking it.
And mocking extract_type(,,) in the same cpp file seems impossible.
Signed-off-by: Johannes Jeising <Johannes.Jeising@de.bosch.com>
Signed-off-by: Johannes Jeising <Johannes.Jeising@de.bosch.com>
There was a problem hiding this comment.
Thanks @jeising. It took me quite a bit to see what's actually happening here, but essentially your last commit and renaming the functions made the deal :)
To be able to use
iceoryxs native applications withros 2based applications the package name defined from service and event name must be valid forros 2as package name. Forcing lowercase in the package name only partially fixes this issue. As far as I know there are changes planned regarding types iniceoryx, so I would propose this fix, till we can focus on the new type handling.