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

Feature key value array #38

Merged
merged 8 commits into from
May 10, 2022

Conversation

bjv-capra
Copy link
Collaborator

This PR adds the ability to have multiple Key values per task, instead of a 1:1 case. An example case would be one task that iterates over multiple errors and needs to push them all at once.

@bjv-capra bjv-capra force-pushed the feature/key-value-array branch from f42f85f to b890dbd Compare May 3, 2022 11:13
Copy link
Contributor

@ralph-lange ralph-lange left a comment

Choose a reason for hiding this comment

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

Nice improvement. I have only minor comments.

@ralph-lange
Copy link
Contributor

I guess micro_ros_diagnostic_msgs/.gitignore can be removed from this PR.

bjv-capra added 4 commits May 4, 2022 05:00
Signed-off-by: Bart Jimenez Vera <[email protected]>
Signed-off-by: Bart Jimenez Vera <[email protected]>
Signed-off-by: Bart Jimenez Vera <[email protected]>
Signed-off-by: Bart Jimenez Vera <[email protected]>
@bjv-capra
Copy link
Collaborator Author

I'll have to force push, as I deleted the commit where I bumped the version

@bjv-capra bjv-capra force-pushed the feature/key-value-array branch from 5109056 to ad9fd4e Compare May 4, 2022 05:09
@bjv-capra
Copy link
Collaborator Author

This pr fixes #26 as well

@codecov-commenter
Copy link

codecov-commenter commented May 4, 2022

Codecov Report

Merging #38 (3fbd246) into master (72ab7f8) will decrease coverage by 1.04%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master     #38      +/-   ##
=========================================
- Coverage    9.58%   8.53%   -1.05%     
=========================================
  Files          27      33       +6     
  Lines        2149    2577     +428     
  Branches      305     291      -14     
=========================================
+ Hits          206     220      +14     
- Misses       1800    2272     +472     
+ Partials      143      85      -58     
Impacted Files Coverage Δ
...detail/micro_ros_diagnostic_status__type_support.c 19.35% <0.00%> (-30.65%) ⬇️
...agnostic_msgs/msg/_micro_ros_diagnostic_status_s.c 0.00% <0.00%> (ø)
...msg/detail/micro_ros_diagnostic_status__struct.hpp 0.00% <0.00%> (ø)
...ro_ros_diagnostic_msgs_s.ep.rosidl_typesupport_c.c 0.00% <0.00%> (ø)
...agnostic_msgs_s.ep.rosidl_typesupport_fastrtps_c.c 0.00% <0.00%> (ø)
...tic_msgs_s.ep.rosidl_typesupport_introspection_c.c 0.00% <0.00%> (ø)
...tail/micro_ros_diagnostic_status__type_support.cpp 0.00% <0.00%> (ø)
...rtps/micro_ros_diagnostic_status__type_support.cpp 0.00% <0.00%> (ø)
...stics/micro_ros_common_diagnostics/src/hwmonitor.c
...stics/micro_ros_common_diagnostics/src/hwmonitor.c
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72ab7f8...3fbd246. Read the comment docs.

Copy link
Contributor

@ralph-lange ralph-lange left a comment

Choose a reason for hiding this comment

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

LGTM. Minor comment: Maybe rename number_of_key_values to number_of_values for consistency with the name of the field in the message type.

Signed-off-by: Bart Jimenez Vera <[email protected]>
@bjv-capra
Copy link
Collaborator Author

I'll do some testing before merging, to ensure I didn't miss anything :)

bjv-capra added 2 commits May 4, 2022 11:13
Signed-off-by: Bart Jimenez Vera <[email protected]>
Signed-off-by: Bart Jimenez Vera <[email protected]>
@bjv-capra bjv-capra merged commit be37125 into micro-ROS:master May 10, 2022
@bjv-capra
Copy link
Collaborator Author

Merging as it fails due to missing rolling packages.

@bjv-capra bjv-capra deleted the feature/key-value-array branch May 10, 2022 08:59
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.

3 participants