-
Notifications
You must be signed in to change notification settings - Fork 91
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
Support parsing elements that are not part of the schema #638
Conversation
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
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.
Looks great! I have some comments to help with the CI failure.
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
src/Param_TEST.cc
Outdated
@@ -75,11 +77,6 @@ TEST(Param, Bool) | |||
strParam.Get<bool>(value); | |||
EXPECT_TRUE(value); | |||
|
|||
// Anything other than 1 or true is treated as a false value |
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.
Is there a reason for removing this test?
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.
Since this section was removed:
https://github.com/ignitionrobotics/sdformat/blob/f3dbe1d225939ff1bc038b0789fff884d763cfa7/include/sdf/Param.hh#L296-L315
It now reaches this error: https://github.com/ignitionrobotics/sdformat/blob/f0f9759ec9d4572db9c5fd9e0dd861e6f679a7cf/src/Param.cc#L329
To me, it made more sense to error when a random value such as %
was set rather than defaulting to false
. Should I change this back?
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.
Yes, I think making it an error makes sense, but I'd like to avoid changing behavior since this is a stable version and someone wrote a test to deliberately check this behavior. Would it be possible to assign the value regardless of the error? I know printing the error is still a change in behavior but I think it's small enough not to break anything.
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.
How's this? 650b544
This seemed to be the easiest fix. Changing ParamPrivate::ValueFromStringImpl
bool section to assign the value regardless of the error and return true (since _valueToSet
was successfully set) caused Param.InvalidBool
test to fail
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.
That works. Can you add a comment there saying that the piece of code handling bool types is there to keep backward behavior?
I'm thinking we should change the behavior for Fortress. Would you agree?
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.
I agree. I'll add the comment and put a TODO
to remove for Fortress
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.
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
🦟 Bug fix
Fixes #603
Summary
sdf::Param::Get
parses a value to a given type regardless of thetypeName
of thesdf::Param
. Previously whentypeName = "string"
thensdf::Param::Get<double>("inf")
would return0
, now it returns the expected valueinf
.Test:
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge