Skip to content

add add_on and remove_on fn to deprecate set_on_param fn#1134

Merged
claireyywang merged 5 commits intomasterfrom
claire/add_missing_fn_lifecyle_node
May 26, 2020
Merged

add add_on and remove_on fn to deprecate set_on_param fn#1134
claireyywang merged 5 commits intomasterfrom
claire/add_missing_fn_lifecyle_node

Conversation

@claireyywang
Copy link
Copy Markdown
Contributor

@claireyywang claireyywang commented May 26, 2020

Part of the effort to deprecate set_on_parameters_set_callback function in #1123 . It adds add_on_set_parameters_callback and remove_on_set_parameters_callback to lifecycle_node

@claireyywang
add add_on and remove_on
b3e526c 
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang
add }
9de9c46 
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang claireyywang added the in review Waiting for review (Kanban column) label May 26, 2020
@claireyywang claireyywang self-assigned this May 26, 2020
@claireyywang
Copy link
Copy Markdown
Contributor Author

claireyywang commented May 26, 2020 •
edited
Loading

CI on all ros2 packages
linux Build Status
linux-aarch64 Build Status
osx Build Status
windows Build Status

@claireyywang claireyywang requested a review from jacobperron May 26, 2020 19:50
@claireyywang claireyywang mentioned this pull request May 26, 2020
Merged
jacobperron
jacobperron approved these changes May 26, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM pending CI

One minor comment.

@claireyywang
remove return from void fn
63a48a1 
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
jacobperron
jacobperron reviewed May 26, 2020
View reviewed changes
@claireyywang
revert
e76a1bb 
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang
reduce line length
223aeec 
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang
Copy link
Copy Markdown
Contributor Author

claireyywang commented May 26, 2020

CI up-to rclcpp_lifecycle
linux Build Status
linux-aarch64 Build Status
osx Build Status
windows Build Status

jacobperron
jacobperron approved these changes May 26, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM (looks like we have the same CI failures on our nightlies)

@claireyywang claireyywang merged commit 01ec06d into master May 26, 2020
@delete-merged-branch delete-merged-branch bot deleted the claire/add_missing_fn_lifecyle_node branch May 26, 2020 22:35
@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented May 27, 2020

@claireyywang This seem to have introduced some failures on nightlies https://ci.ros2.org/view/nightly/job/nightly_linux_release/1555/#showFailuresLink.

Psst: We usually use squash merge instead of merge.

@ivanpauno ivanpauno mentioned this pull request May 27, 2020
Merged
@jacobperron
Copy link
Copy Markdown
Member

jacobperron commented May 27, 2020

@ivanpauno Can you point to the specific failure? It looks like the same failures have been occurring since May 23: https://ci.ros2.org/view/nightly/job/nightly_linux_release/1551/

@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented May 27, 2020

@ivanpauno Can you point to the specific failure? It looks like the same failures have been occurring since May 23: https://ci.ros2.org/view/nightly/job/nightly_linux_release/1551/

Yeap, sorry for the noise.
I will look for the actual root cause of the issue.

@Karsten1987 Karsten1987 mentioned this pull request May 27, 2020
Merged
@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented May 27, 2020

Psst: We usually use squash merge instead of merge.

Yeah, please use squash in the future. It makes cherry-picking to different distributions so much easier. <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjwwood wjwwood wjwwood approved these changes

@jacobperron jacobperron jacobperron approved these changes

Assignees

@claireyywang claireyywang

Labels

in review Waiting for review (Kanban column)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@claireyywang @ivanpauno @jacobperron @wjwwood