Fix ambiguous constructor overload

[christophfroehlich]

On jazzy and before the cleanup #494, we had an ambiguous overload. I suggest changing the same on kilted+rolling to have the same API everywhere.

docstring was wrong anyways 🙈

/workspaces/ros2_jazzy_ws/src/control_toolbox/control_toolbox/test/pid_ros_parameters_tests.cpp: In instantiation of ‘TestablePidROS::TestablePidROS(std::shared_ptr<_Tp>, std::string) [with NodeT = rclcpp::Node; std::string = std::__cxx11::basic_string<char>]’:
/workspaces/ros2_jazzy_ws/src/control_toolbox/control_toolbox/test/pid_ros_parameters_tests.cpp:136:30:   required from here
/workspaces/ros2_jazzy_ws/src/control_toolbox/control_toolbox/test/pid_ros_parameters_tests.cpp:50:51: error: call of overloaded ‘PidROS(std::shared_ptr<rclcpp::Node>&, std::string&)’ is ambiguous
   50 |   : control_toolbox::PidROS(node_ptr, param_prefix)
      |                                                   ^
In file included from /workspaces/ros2_jazzy_ws/src/control_toolbox/control_toolbox/test/pid_ros_parameters_tests.cpp:16:
/workspaces/ros2_jazzy_ws/src/control_toolbox/control_toolbox/include/control_toolbox/pid_ros.hpp:96:12: note: candidate: ‘control_toolbox::PidROS::PidROS(std::shared_ptr<_Tp>, const std::string&, const std::string&) [with NodeT = rclcpp::Node; std::string = std::__cxx11::basic_string<char>]’
   96 |   explicit PidROS(
      |            ^~~~~~
/workspaces/ros2_jazzy_ws/src/control_toolbox/control_toolbox/include/control_toolbox/pid_ros.hpp:71:89: note: candidate: ‘control_toolbox::PidROS::PidROS(std::shared_ptr<_Tp>, std::string, bool) [with NodeT = rclcpp::Node; std::string = std::__cxx11::basic_string<char>]’
   71 |   [[deprecated("Use overloads with explicit prefixes for params and topics")]] explicit PidROS(
      |                                                                                         ^~~~~~

[saikishor]

Looks good to me, but I'm afraid that it might break many packages. Should we deprecate it before?

Let's deprecate this here and the other one in Jazzy? Or let's remove as you proposed

[christophfroehlich]

It was not working before the cleanup #494 anyways, right? The working version was just released a few days ago; and there is no use of this class on the ros buildfarm afaik. I don't see a high risk here

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.35%. Comparing base (6829ff1) to head (180b42e).

Additional details and impacted files

@@             Coverage Diff              @@
##           ros2-master     #499   +/-   ##
============================================
  Coverage        82.35%   82.35%           
============================================
  Files               29       29           
  Lines             1984     1984           
  Branches           114      114           
============================================
  Hits              1634     1634           
  Misses             281      281           
  Partials            69       69           

Flag	Coverage Δ
unittests	82.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ontrol_toolbox/include/control_toolbox/pid_ros.hpp	100.00% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[saikishor]

It was not working before the cleanup #494 anyways, right? The working version was just released a few days ago; and there is no use of this class on the ros buildfarm afaik. I don't see a high risk here

These are the ambiguous constructors that can accept 2 args in Jazzy:

control_toolbox/control_toolbox/include/control_toolbox/pid_ros.hpp

Lines 70 to 87 in 7b819ce

	template <class NodeT>
	[[deprecated("Use overloads with explicit prefixes for params and topics")]] explicit PidROS(
	std::shared_ptr<NodeT> node_ptr, std::string prefix = std::string(""),
	bool prefix_is_for_params = false)
	: PidROS(
	node_ptr->get_node_base_interface(), node_ptr->get_node_logging_interface(),
	node_ptr->get_node_parameters_interface(), node_ptr->get_node_topics_interface(), prefix,
	prefix_is_for_params)
	{
	}
	template <class NodeT>
	explicit PidROS(std::shared_ptr<NodeT> node_ptr, const std::string & param_prefix)
	: PidROS(
	node_ptr->get_node_base_interface(), node_ptr->get_node_logging_interface(),
	node_ptr->get_node_parameters_interface(), node_ptr->get_node_topics_interface(),
	param_prefix, "", false)
	{
	}

This is the only constructor that accepts 2 args in Rolling:

control_toolbox/control_toolbox/include/control_toolbox/pid_ros.hpp

Lines 70 to 77 in 6829ff1

	template <class NodeT>
	explicit PidROS(std::shared_ptr<NodeT> node_ptr, const std::string & param_prefix)
	: PidROS(
	node_ptr->get_node_base_interface(), node_ptr->get_node_logging_interface(),
	node_ptr->get_node_parameters_interface(), node_ptr->get_node_topics_interface(),
	param_prefix, "", false)
	{
	}

What I mean is, if we remove the only constructor that is using the 2 args, then if some user code has this change, it will break his code. Does it make sense?. If so, what I'm saying is better to add a deprecation note to the 2 args constructor and then remove current non-deprecated constructor in Jazzy

[christophfroehlich]

It was not working before the cleanup #494 anyways, right? The working version was just released a few days ago; and there is no use of this class on the ros buildfarm afaik. I don't see a high risk here

These are the ambiguous constructors that can accept 2 args in Jazzy:

control_toolbox/control_toolbox/include/control_toolbox/pid_ros.hpp

Lines 70 to 87 in 7b819ce

	template <class NodeT>
	[[deprecated("Use overloads with explicit prefixes for params and topics")]] explicit PidROS(
	std::shared_ptr<NodeT> node_ptr, std::string prefix = std::string(""),
	bool prefix_is_for_params = false)
	: PidROS(
	node_ptr->get_node_base_interface(), node_ptr->get_node_logging_interface(),
	node_ptr->get_node_parameters_interface(), node_ptr->get_node_topics_interface(), prefix,
	prefix_is_for_params)
	{
	}
	template <class NodeT>
	explicit PidROS(std::shared_ptr<NodeT> node_ptr, const std::string & param_prefix)
	: PidROS(
	node_ptr->get_node_base_interface(), node_ptr->get_node_logging_interface(),
	node_ptr->get_node_parameters_interface(), node_ptr->get_node_topics_interface(),
	param_prefix, "", false)
	{
	}

but this is not functional with the error message above. this was "accidentally" fixed with #494 (5.8.0 on rolling), released just a couple of days ago:

This is the only constructor that accepts 2 args in Rolling:

control_toolbox/control_toolbox/include/control_toolbox/pid_ros.hpp

Lines 70 to 77 in 6829ff1

	template <class NodeT>
	explicit PidROS(std::shared_ptr<NodeT> node_ptr, const std::string & param_prefix)
	: PidROS(
	node_ptr->get_node_base_interface(), node_ptr->get_node_logging_interface(),
	node_ptr->get_node_parameters_interface(), node_ptr->get_node_topics_interface(),
	param_prefix, "", false)
	{
	}

What I mean is, if we remove the only constructor that is using the 2 args, then if some user code has this change, it will break his code. Does it make sense?. If so, what I'm saying is better to add a deprecation note to the 2 args constructor and then remove current non-deprecated constructor in Jazzy

This means that after #431 (5.6.0 on rolling, 4.6.0 on jazzy) it was not possible to use the 2-args constructor. So I don't think that anyone used it (no one will have used it since the 5.8.0 release this week). That's why I don't think this hurts if we remove it now.

Pre-release job only fails because of this strange linter fail of gz_ros2_control_demos.

[saikishor]

Understood. I see that the 2 arg constructor is recently added.
Thanks for the clarification buddy