Skip to content

compare the return value to constants#665

Closed
iuhilnehc-ynos wants to merge 2 commits intoros2:masterfrom
iuhilnehc-ynos:topic-compare-to-constants
Closed

compare the return value to constants#665
iuhilnehc-ynos wants to merge 2 commits intoros2:masterfrom
iuhilnehc-ynos:topic-compare-to-constants

Conversation

@iuhilnehc-ynos
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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