Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid name clashing in ros msg packages #609

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Affonso-Gui
Copy link
Member

Solves #574.

@Affonso-Gui
Copy link
Member Author

#574 (comment)

There are a few possible solutions for this:

  • Do not use the lisp package on ros message packages
  • Require necessary messages upon loading
  • Make sure that the symbol belongs to the designed package during operation

Adopting the first one for simplicity and to avoid re-generating the messages. Since no coding is performed from inside the ros message packages this should have no side effects (tests will confirm that for us).

Affonso-Gui added a commit to Affonso-Gui/jsk_roseus that referenced this pull request Mar 27, 2019
@Affonso-Gui
Copy link
Member Author

Added test code

@k-okada
Copy link
Member

k-okada commented Apr 1, 2019

roseus does not depend on jsk_recognition_msgs, so I think it is better to create message for test code, like https://github.com/jsk-ros-pkg/jsk_roseus/pull/509/files.
I also prefer to commit test code first, and confirm the travis failed, then commit the patch (#567)

@Affonso-Gui
Copy link
Member Author

@k-okada Thanks, I was insecure about the previous test code, and the link was of great help.
I added the tests to the existing test-name* tests since

  1. Both of them test for name clashing in ros messages
  2. Want to avoid having to create new test messages

Test code is also added in #610

@knorth55
Copy link
Member

👍 for this PR.

@Affonso-Gui
Copy link
Member Author

@knorth55 Actually we had some test failures and possible bugs here. I'll try to have a look on this week and make any necessary changes to reactivate this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants