There appear to be more instances of warnFrameId that could pass through the error message instead of just printing to console.

This logic could be worked inside of warnFrameId instead of replacing it. Then it could be reused elsewhere. The prototype would need to be extended with the error_msg pointer.

That sounds like a great idea for a followup PR by someone who is more comfortable than I with this code

This logic is creating the warnings, throwing an exception then immediately catching the exception and then pulling the string out of it. This is in one of the potentially highest throughput and consequently desired to be lowest latency sections of code. We don't want to be doing that.

If we add the third check of non-existant frame to warnFrameId it will have full coverage of the cases for validateFrameId's cases. And right now warnFrameId only outputs to console. In some cases like can_transform where we have an error string that we can populate if it's set. We should add that same capability to warnFrameId by adding the error_msg argument and then it can simply be added as an additional argument. And the behavior can be to either populate that error_msg if it's present, otherwise do the current behavior and send a warning to the console.

This will be an overloaded function and the current prototype can be updated to simply call the new version with the error_msg set to null to get the same behavior.

This logic would be great for a helper function that converts the enum to string to keep the complexity of the functions lower. It then could also have a small unit test to ensure coverage too.

This function exists just for logging. It's appropriate to convert the enum here. Also, I have no idea what the enums actually mean beyond what their names say.

Just because it's used for logging doesn't mean it shouldn't have coverage and follow best practices.

The code would be much more readable if the whole case block was just a function std::string get_failure_reason_string(const FilterFailureReason& reason)

That then is invoked in the RCLCPP_INFO element instead of reason_string.c_str() it would be get_failure_reason_string(reason).c_str() And this method stays 5 lines long with no branchpoints.

And a unit test can be written for the static function that exercises all 3 cases and provides full coverage of that function.

This is just cleaning up the structure of the code to avoid building up technical debt.

Just because it's used for logging doesn't mean it shouldn't have coverage and follow best practices.

The code would be much more readable if the whole case block was just a function std::string get_failure_reason_string(const FilterFailureReason& reason)

That then is invoked in the RCLCPP_INFO element instead of reason_string.c_str() it would be get_failure_reason_string(reason).c_str() And this method stays 5 lines long with no branchpoints.

And a unit test can be written for the static function that exercises all 3 cases and provides full coverage of that function.

This is just cleaning up the structure of the code to avoid building up technical debt.

This logic is creating the warnings, throwing an exception then immediately catching the exception and then pulling the string out of it. This is in one of the potentially highest throughput and consequently desired to be lowest latency sections of code. We don't want to be doing that.

If we add the third check of non-existant frame to warnFrameId it will have full coverage of the cases for validateFrameId's cases. And right now warnFrameId only outputs to console. In some cases like can_transform where we have an error string that we can populate if it's set. We should add that same capability to warnFrameId by adding the error_msg argument and then it can simply be added as an additional argument. And the behavior can be to either populate that error_msg if it's present, otherwise do the current behavior and send a warning to the console.

This will be an overloaded function and the current prototype can be updated to simply call the new version with the error_msg set to null to get the same behavior.