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

Add changes to enable searching for .msg files in sub-directories #1218

Open
wants to merge 7 commits into
base: rolling
Choose a base branch
from

Conversation

aditya2592
Copy link

@aditya2592 aditya2592 commented Dec 19, 2022

Fix for #1188

@aditya2592 aditya2592 marked this pull request as ready for review December 19, 2022 08:19
@aditya2592 aditya2592 requested a review from a team as a code owner December 19, 2022 08:19
@aditya2592 aditya2592 requested review from emersonknapp and hidmic and removed request for a team December 19, 2022 08:19
@aditya2592 aditya2592 changed the title Add Changes to enable searching for .msg files in sub-directories Add changes to enable searching for .msg files in sub-directories Dec 19, 2022
Signed-off-by: aditya <aditya@nimble.ai>
Signed-off-by: aditya <aditya@nimble.ai>
@aditya2592
Copy link
Author

Failure in this test seems unrelated to the change in this PR

@james-rms
Copy link
Contributor

Are you able to add a test for this new behavior?

Copy link
Contributor

@james-rms james-rms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include some equivalent of this patch to prevent the message definition cache mistaking CompressedImage.msg for Image.msg. Also, please add a test to this repo to ensure that this behavior is correct going forward.

Signed-off-by: aditya <aditya@nimble.ai>
@aditya2592
Copy link
Author

Please include some equivalent of this patch to prevent the message definition cache mistaking CompressedImage.msg for Image.msg. Also, please add a test to this repo to ensure that this behavior is correct going forward.

Patch has been added. Is there any relevant documentation to compile and run tests in this repo? Or should I follow the normal colcon test process? Also is there a suggested test file to add it to?

@james-rms
Copy link
Contributor

test_message_definition_cache.cpp contains tests for this component, and includes several instances of searching for message definitions within the rosbag2_storage_mcap_testdata package. I'd recommend adding a nested subdirectory definition to the testdata package and testing that it can be found.

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

Successfully merging this pull request may close these issues.

None yet

3 participants