-
Notifications
You must be signed in to change notification settings - Fork 5
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
Removed SearchMonitor #46
Conversation
To be tested on a working setup |
At first sight, judging from the changes, we are losing a whole functionality. |
It's not clear to me if that code is manipulating config parameters (*), adding default values, or just printing help messages. Do you have a setup where it can be tested? (*) in this case some changes have be applied in the .ini file. |
I've no memory of that functionality: it was developed long long time ago. Probably, it may help to know how the functionalities offered by the methods getMonitor() / setMonitor() are remapped. |
Ideally, there's a dedicated test as suggested by @traversaro: Need to check if it's still working though. |
I am currently trying to do that, I will keep you posted. |
I'm still not 100% sure but I think that the only purpose of that code is to print the values read from the configuration file:
Or, if the parameters are not found, to print the default value that it will be used:
|
Thank you @randaz81 for doing the test 👍🏻
I've also double-checked the code and we should be safe removing it as @randaz81 is right about its purpose. For now, there's no equivalent functionality that has an easy drop-in replacement as the conf files would need to be refactored to rely on the new way (see robotology/yarp#3059). All considered, we can proceed with merging. |
Thanks @pattacini @randaz81 ! |
A fix for #45