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

adapt to the NULL removal from rmw result validation string #157

Merged
merged 3 commits into from
Jan 8, 2018

Conversation

gaoethan
Copy link
Contributor

@gaoethan gaoethan commented Nov 28, 2017

Change to align to the update of APIs rmw_*_validation_result_string()
Signed-off-by: Ethan Gao ethan.gao@linux.intel.com

Connects to ros2/rmw#130

@gaoethan
Copy link
Contributor Author

This PR relates to PR: #ros2/rmw#130

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

All these functions should not raise as their goal is to return the error message. That's why they were raising only on NULL return (unknown error message).

So if we decide to not return NULL on "unknown error" we should just delete these if blocks completely.

If we go back to return NULL on unknown error code, we should not change this file at all.

@gaoethan
Copy link
Contributor Author

gaoethan commented Dec 5, 2017

yes, I initially intend to show the specific validation message, but the error msg is returned from the final result_list, there is no need now, thanks for your comments.

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

It may be worth keeping this check as it will allow us to keep a good fallback behavior:

  • If the X_NAME_IS_VALID we return None without raising
  • If the validation error string is not NULL we return it
  • If it's NULL: Something is wrong (that should never happen) so we raise and return NULL as C/Python extensions must do when they raise.

Sorry for the back and forth on this but I believe that this PR is not necessary.

Edit: Actually it doesn't make sense to test for something that by design will never happen so let's not check it in any of the client libraries 👍 let's keep this PR as is

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

thanks @gaoethan for iterating

Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>
Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>
because the unknown case doesn't return NULL now

Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>
@mikaelarguedas mikaelarguedas merged commit ff5fbe1 into ros2:master Jan 8, 2018
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

2 participants