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

compare the return value to constants #665

Closed

Conversation

iuhilnehc-ynos
Copy link
Collaborator

Signed-off-by: Chen Lihui Lihui.Chen@sony.com

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>
if (logging_status != 0) {
status = RCL_RET_ERROR;
status = rcl_logging_external_set_logger_level(NULL, default_level);
if (RCL_RET_OK == status) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changes a logic, only when log level can be set, then register handler?
Previously registering handler anyway even if log level cannot be set.

Copy link
Collaborator Author

@iuhilnehc-ynos iuhilnehc-ynos May 28, 2020

Choose a reason for hiding this comment

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

Yes. And I think this is the correct logic. Maybe I was wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fujitatomoya
After consideration, I shouldn't update the original logic while refactoring the source code.
I will update it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@iuhilnehc-ynos

I guess that original logic is meant to, I'd like to hear from others.

Signed-off-by: Chen Lihui <Lihui.Chen@sony.com>
@clalancette
Copy link
Contributor

I'd actually like to hold off on a change like this. Over in ros2/rcl_logging#33 (comment) , we've been discussing refactoring this interface, and the types that rcl_logging_external_set_logger_level() will return will almost certainly change.

We aren't working on it at the moment, but feel free to start working on that if you'd like. Otherwise, I will probably start taking a look at fixing this up after Foxy is released.

@iuhilnehc-ynos
Copy link
Collaborator Author

I'd actually like to hold off on a change like this. Over in ros2/rcl_logging#33 (comment) , we've been discussing refactoring this interface, and the types that rcl_logging_external_set_logger_level() will return will almost certainly change.

Thanks for your explanation. I got it and close this PR right now.

We aren't working on it at the moment, but feel free to start working on that if you'd like. Otherwise, I will probably start taking a look at fixing this up after Foxy is released.

Actually, I updated this function because of #664 ( I noticed there is a TODO inside rcl_logging_configure_with_output_handler).
If I have time, I'd like to do that.

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