-
Notifications
You must be signed in to change notification settings - Fork 104
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
Bugfix kbemf
set to 0 by default when not found
#935
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks heaps for the fix @sgiraz
Just one inline comment to address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Awaiting the CI before merging.
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Is kbemf not used anymore? Then it's ok to have a default = 0. Otherwise I'd like to say that's a bad software practice to mix mandatory with optional parameters in the same parser block. In the long term things get confused, undocumented and In the past I have seen default value changing without any notice, leading to difficult-to-debug situations. For this reason, at least in yarp, the use default parameters is generally discouraged, and a revision of the configuration files to add the missing parameter is preferred. |
Hi @randaz81
Indeed, |
This PR enables the
eomcParser
to parse all parameters withinTRQ_PID_OUTPUT_CURR
, even ifkbemf
is missing.cc @pattacini @Nicogene @isorrentino @DanielePucci