Skip to content
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

Fix Element Set method return value #1256

Merged
merged 2 commits into from
Mar 31, 2023
Merged

Fix Element Set method return value #1256

merged 2 commits into from
Mar 31, 2023

Conversation

marcoag
Copy link
Member

@marcoag marcoag commented Mar 21, 2023

🦟 Bug fix

Summary

Discovered here, the Element Set method would return true ignoring the result coming from the this->dataPtr->value->Set(_value);. This could cause the Set method to sometimes error but still return true.

This PR fixes it and it makes the method return the result of this->dataPtr->value->Set(_value); instead of the hardcoded true.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@osrf-triage osrf-triage added this to Inbox in Core development Mar 21, 2023
@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏰 citadel Ignition Citadel labels Mar 21, 2023
@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

❗ No coverage uploaded for pull request base (sdf9@11ad259). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head ddc7126 differs from pull request most recent head 9b196ce. Consider uploading reports for the commit 9b196ce to get more accurate results

@@           Coverage Diff           @@
##             sdf9    #1256   +/-   ##
=======================================
  Coverage        ?   88.35%           
=======================================
  Files           ?       63           
  Lines           ?    10092           
  Branches        ?        0           
=======================================
  Hits            ?     8917           
  Misses          ?     1175           
  Partials        ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@azeey
Copy link
Collaborator

azeey commented Mar 21, 2023

@scpeters Do you think this is safe for Gazebo-classic?

@azeey azeey moved this from Inbox to In review in Core development Mar 27, 2023
@scpeters
Copy link
Member

@scpeters Do you think this is safe for Gazebo-classic?

I looked and I don't see a single place where the return value of Element::Set is checked. So that's maybe concerning on its own, but it means that this pull request shouldn't affect gazebo-classic. Thanks for checking

@marcoag
Copy link
Member Author

marcoag commented Mar 31, 2023

@osrf-jenkins run tests please

@azeey azeey merged commit 133bc87 into sdf9 Mar 31, 2023
Core development automation moved this from In review to Done Mar 31, 2023
@azeey azeey deleted the fix_element_set_return branch March 31, 2023 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel Gazebo 1️1️ Dependency of Gazebo classic version 11
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants