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

Remove unused on_parameters_set_callback_ #1945

Merged

Conversation

nnmm
Copy link
Contributor

@nnmm nnmm commented Jun 6, 2022

I could be wrong, but this callback seems to be obsolete – there is no way to set it through the public API.

Signed-off-by: Nikolai Morin <nikolai.morin@apex.ai>
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.

the change looks good to me.

#772 allows it to register multiple on_parameters_set_callback.
I am not sure why on_parameters_set_callback_ is removed at the same time...

@ivanpauno any thoughts?

@alsora
Copy link
Collaborator

alsora commented Jun 10, 2022

Looks good to me.
The on_parameters_set_callback_ appears to have been replaced by on_parameters_set_callback_container_ which is set through add_on_set_parameters_callback

@fujitatomoya
Copy link
Collaborator

CI:

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

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!

@ivanpauno
Copy link
Member

I am not sure why on_parameters_set_callback_ is removed at the same time...

@ivanpauno any thoughts?

There was a tick-tock cycle.
We forgot removing this in #1199.

@ivanpauno ivanpauno added the enhancement New feature or request label Jun 14, 2022
@ivanpauno ivanpauno merged commit 86a9d58 into ros2:master Jun 14, 2022
@ivanpauno
Copy link
Member

Thanks @nnmm !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants