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.

Sign up for GitHub

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

Jump to bottom

to create a sublogger while getting child of Logger #1717

Merged
merged 5 commits into from Feb 24, 2023
Merged

to create a sublogger while getting child of Logger #1717

merged 5 commits into from Feb 24, 2023

Conversation

iuhilnehc-ynos
Copy link
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos commented Jul 16, 2021 •
edited

use new APIs based on ros2/rcl#921

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

@fujitatomoya fujitatomoya self-requested a review July 17, 2021 01:32
fujitatomoya
fujitatomoya approved these changes Jul 22, 2021
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

a minor comment, but LGTM.

rclcpp/include/rclcpp/logger.hpp Outdated Show resolved Hide resolved
@clalancette clalancette self-assigned this Aug 12, 2021
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:21
@fujitatomoya
Copy link
Collaborator

@iuhilnehc-ynos can you start CI with ros2/rcl#921?

@iuhilnehc-ynos
Copy link
Collaborator Author

@fujitatomoya

I need to re-check if the implementations are good or not, after that, I'll trigger the CI.

Chen Lihui added 3 commits December 8, 2022 11:53
to create a sublogger while getting child of Logger
640a479 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
add tests for get_child
66d5ee4 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
remove comment
67394c3 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@iuhilnehc-ynos iuhilnehc-ynos force-pushed the topic-get-child-to-create-sublogger branch from bd4f5a3 to 67394c3 Compare December 8, 2022 03:54
@iuhilnehc-ynos
Copy link
Collaborator Author

iuhilnehc-ynos commented Dec 8, 2022 •
edited

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator

@clalancette @wjwwood i am good to go with this change, could you do another review?

this needs to be aligned with ros2/rcl#921

@iuhilnehc-ynos
Copy link
Collaborator Author

iuhilnehc-ynos commented Dec 9, 2022 •
edited

The URL for Windows by https://ci.ros2.org/job/ci_launcher/11246/console seems incorrect (Maybe it's because rebuild the ci_launcher before https://ci.ros2.org/job/ci_launcher/11242/console, but the windows in https://ci.ros2.org/job/ci_launcher/11242/console didn't trigger at that time.).

After checking the commit ID in Windows Build Status that belongs to https://ci.ros2.org/job/ci_launcher/11242/console, although it is correct, I'd like to re-run CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator

@clalancette could you also review this? this depends on ros2/rcl#921, they must be merged together.

@fujitatomoya fujitatomoya mentioned this pull request Feb 14, 2023
Closed
clalancette
clalancette requested changes Feb 15, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've got one thing that I think can be simplified; otherwise, this looks pretty good to me.

rclcpp/include/rclcpp/logger.hpp Show resolved Hide resolved
add more test cases and remove checking RCL_RET_SUBLOGGER_ALREADY_EXIST
0697284 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
clalancette
clalancette requested changes Feb 17, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Two small fixes here, then I think this will be good.

rclcpp/src/rclcpp/logger.cpp Outdated Show resolved Hide resolved
rclcpp/test/rclcpp/CMakeLists.txt Outdated Show resolved Hide resolved
address review
48e13f1 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@iuhilnehc-ynos iuhilnehc-ynos mentioned this pull request Feb 24, 2023
Merged
@iuhilnehc-ynos
Copy link
Collaborator Author

@ros-pull-request-builder retest this please

1 similar comment
@iuhilnehc-ynos
Copy link
Collaborator Author

@ros-pull-request-builder retest this please

@iuhilnehc-ynos
Copy link
Collaborator Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

The single failed test on Windows is a known flake, so going ahead and merging this.

@clalancette clalancette merged commit b1e834a into ros2:rolling Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@clalancette clalancette clalancette approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno ivanpauno is a code owner

@hidmic hidmic Awaiting requested review from hidmic hidmic is a code owner

@wjwwood wjwwood Awaiting requested review from wjwwood wjwwood is a code owner

Assignees

@clalancette clalancette

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@iuhilnehc-ynos @fujitatomoya @clalancette