Skip to content

Remove unused on_parameters_set_callback_#1945

Merged
ivanpauno merged 1 commit intoros2:masterfrom
ApexAI:remove-unused-on-parameters-set-callback
Jun 14, 2022
Merged

Remove unused on_parameters_set_callback_#1945
ivanpauno merged 1 commit intoros2:masterfrom
ApexAI:remove-unused-on-parameters-set-callback

Conversation

@nnmm
Copy link
Copy Markdown
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
Copy Markdown
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?

@fujitatomoya fujitatomoya requested a review from ivanpauno June 8, 2022 20:35
@alsora
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

CI:

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

4 participants