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

Add Element::FindElement as an alternative to Element::GetElement #620

Merged
merged 3 commits into from
Jul 9, 2021

Conversation

azeey
Copy link
Collaborator

@azeey azeey commented Jul 9, 2021

🎉 New feature

Closes #188

Summary

Element::GetElement may create a child element if it doesn't exist.
This makes it a non-const function. Instead of changing this behavior, a
new function Element::FindElement is added that returns a nullptr if
the child element is not found instead of creating a new element.

Test it

Run test UNIT_Element_TEST.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

`Element::GetElement` may create a child element if it doesn't exist.
This makes it a non-const function. Instead of changing this behavior, a
new function `Element::FindElement` is added that returns a nullptr if
the child element is not found instead of creating a new element.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jul 9, 2021
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omg, this is great!

@@ -825,6 +825,25 @@ TEST(Element, GetNextElementMultiple)
ASSERT_EQ(child2->GetNextElement(""), nullptr);
}

/////////////////////////////////////////////////
/// Helper function to add child elements without having to create descriptions
sdf::ElementPtr AddChildElement(sdf::ElementPtr _parent,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: we usually use camelCase for free functions

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in d519304. I'm never sure what to do about free functions and static functions because I think we diverge from the google style guide on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we don't have this documented anywhere, it's one of those things that we need to document

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey merged commit 8fca2f1 into gazebosim:sdf9 Jul 9, 2021
@chapulina chapulina added this to Inbox in Core development via automation Jul 12, 2021
@chapulina chapulina moved this from Inbox to Highlights in Core development Jul 12, 2021
@jennuine jennuine mentioned this pull request Jul 12, 2021
8 tasks
jennuine pushed a commit that referenced this pull request Jul 29, 2021
`Element::GetElement` may create a child element if it doesn't exist.
This makes it a non-const function. Instead of changing this behavior, a
new function `Element::FindElement` is added that returns a nullptr if
the child element is not found instead of creating a new element.


Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@j-rivero j-rivero removed this from Highlights in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants