-
Notifications
You must be signed in to change notification settings - Fork 181
[YAML Parser] Depend on rcutils only #470
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
Conversation
Signed-off-by: Michel Hidalgo <[email protected]>
ivanpauno
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some minor comments. Besides that, LGTM.
| rcl_params_t * params_st); | ||
|
|
||
| /// \brief Print the parameter structure to stdout | ||
| /// \brief Print the parameter structure to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unintended change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap. Fixed in cf8bff1.
rcl_yaml_param_parser/src/parser.c
Outdated
| break; | ||
| default: | ||
| res = RCL_RET_ERROR; | ||
| ret = RCUTILS_RET_OK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RCL_RET_ERROR -> RCUTILS_RET_OK ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. Fixed in cf8bff1.
rcl_yaml_param_parser/src/parser.c
Outdated
| if (NULL == value) { | ||
| RCUTILS_SET_ERROR_MSG("event argument has no value"); | ||
| return RCUTILS_RET_INVALID_ARGUMENT; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it makes sense. Done in cf8bff1.
rcl_yaml_param_parser/src/parser.c
Outdated
| if (NULL == value) { | ||
| RCUTILS_SET_ERROR_MSG("event argument has no value"); | ||
| return RCUTILS_RET_INVALID_ARGUMENT; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use RCUTILS_CHECK_FOR_NULL_WITH_MSG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in cf8bff1.
rcl_yaml_param_parser/src/parser.c
Outdated
| RCL_SET_ERROR_MSG("YAML file path is NULL"); | ||
| RCUTILS_SET_ERROR_MSG("YAML file path is NULL"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use RCUTILS_CHECK_ARGUMENT_FOR_NULL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in cf8bff1.
Signed-off-by: Michel Hidalgo <[email protected]>
ivanpauno
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! (with green CI)
| RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( | ||
| "The name length exceeds the MAX size %d at line %d", MAX_STRING_SIZE, line_num); | ||
| return RCL_RET_OK; | ||
| ret = RCUTILS_RET_ERROR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RCUTILS_RET_ERROR -> RCL_RET_OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading it again, I think it was wrong before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what I thought.
|
Re-running CI, looks like some code in |
|
The sole CI failure on Linux is due to a flake8 complaint on rclpy, which has since been fixed in ros2/rclpy#385. |
Closes #252. In preparation for pushing parameter parsing (from file and command line arguments) down to
rcl.