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.

Sign up for GitHub

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

Jump to bottom

Refactor interface messages #3

Merged
merged 3 commits into from
May 1, 2019
Merged

Refactor interface messages #3

merged 3 commits into from
May 1, 2019

Conversation

jacobperron
Copy link
Member

  • Added new interfaces for missing combinations of nested of nested types as described in [rosidl_generator_cpp] Use message definitions from the package test_interface_files rosidl#367 (comment). Also, renamed existing combinations for consistency.
  • Renamed interfaces containing other interface types to use the name "Structure" instead of "Nested". I think this is more consistent with IDL naming conventions.
  • Made the size of nested types the same for testing convenience and increased them to 7 to allow for more variety in test values.

Connects to ros2/rcl_interfaces#58
Blocking ros2/rosidl#367
Blocking ros2/rosidl#368

@jacobperron jacobperron self-assigned this Apr 26, 2019
@jacobperron jacobperron requested a review from dirk-thomas April 26, 2019 19:16
@jacobperron jacobperron force-pushed the jacob/update_test_msgs branch 2 times, most recently from c37af55 to 223b778 Compare April 27, 2019 03:26
@jacobperron
Copy link
Member Author

jacobperron commented Apr 27, 2019 •
edited
Loading

@dirk-thomas I've updated this based on our offline discussion.

By grouping more fields together in the same interfaces we've cut down on the total number of interfaces while effectively covering the same test cases.

dirk-thomas
dirk-thomas reviewed Apr 28, 2019
View reviewed changes
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

I like the new message names 👍

Inline a few comments mostly about default values.

@jacobperron jacobperron force-pushed the jacob/update_test_msgs branch from ffbec89 to 6da1125 Compare April 29, 2019 19:44
dirk-thomas
dirk-thomas reviewed Apr 29, 2019
View reviewed changes
jacobperron added 3 commits May 1, 2019 11:31
@jacobperron
Rename messages towards consolidation
dabaec7 
Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Refactor message fields
90b3fd1 
* Add fields to cover test cases
* Move string fields to Strings.msg
* Comment out fields that have issues to be resolved

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Remove old interface files
358b5a8 
Signed-off-by: Jacob Perron <[email protected]>
@jacobperron jacobperron mentioned this pull request May 1, 2019
Merged
@jacobperron jacobperron force-pushed the jacob/update_test_msgs branch from d64d61c to 358b5a8 Compare May 1, 2019 20:43
@jacobperron
Copy link
Member Author

Cleaned up commits.

@jacobperron jacobperron merged commit 23a8330 into master May 1, 2019
@delete-merged-branch delete-merged-branch bot deleted the jacob/update_test_msgs branch May 1, 2019 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas approved these changes

Assignees

@jacobperron jacobperron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jacobperron @dirk-thomas