Skip to content

PrintConfig option to preserve includes when converting to string#749

Merged
jennuine merged 9 commits intosdf12from
jennuine/preserve_includes
Dec 9, 2021
Merged

PrintConfig option to preserve includes when converting to string#749
jennuine merged 9 commits intosdf12from
jennuine/preserve_includes

Conversation

@jennuine
Copy link
Copy Markdown
Collaborator

@jennuine jennuine commented Nov 11, 2021

🎉 New feature

Closes #522

Summary

Added option to preserve <include> tags when converting the element to a string using PrintConfig. By default, the <include> element will be expanded.

Test it

export SDF_PATH=/path/to/sdformat/test/integration/model

# print expanded <include>s
ign sdf -p /path/to/sdformat/test/integration/include_model.sdf

# print preserved <include>s
ign sdf -p -i /path/to/sdformat/test/integration/include_model.sdf

or

./path/to/build/test/integration/INTEGRATION_print_config

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

@jennuine
add option to preserve includes
2914ecd 
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@jennuine jennuine requested a review from azeey November 11, 2021 23:57
@github-actions github-actions bot added 🌱 garden Ignition Garden 🏯 fortress Ignition Fortress labels Nov 11, 2021
@azeey azeey requested a review from marcoag November 12, 2021 00:02
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 12, 2021 •
edited
Loading

Codecov Report

Merging #749 (0e84667) into sdf12 (e8a73e4) will decrease coverage by 0.10%.
The diff coverage is 37.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##            sdf12     #749      +/-   ##
==========================================
- Coverage   89.40%   89.30%   -0.11%     
==========================================
  Files          76       76              
  Lines       12326    12347      +21     
==========================================
+ Hits        11020    11026       +6     
- Misses       1306     1321      +15
Impacted Files Coverage Δ
src/ign.cc 62.37% <0.00%> (-10.88%) ⬇️
src/Element.cc 96.86% <100.00%> (+<0.01%) ⬆️
src/PrintConfig.cc 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8a73e4...0e84667. Read the comment docs.

@azeey azeey removed their request for review November 16, 2021 17:11
marcoag
marcoag reviewed Nov 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@marcoag marcoag left a comment

Choose a reason for hiding this comment

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

Great job, this is nice!

Just one minor suggestion, the rest works great and looks good to me.

include/sdf/PrintConfig.hh Outdated

/// \brief Private data pointer.
IGN_UTILS_IMPL_PTR(dataPtr)
};
Copy link
Copy Markdown
Member

@marcoag marcoag Nov 17, 2021

Choose a reason for hiding this comment

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

nit: not really directly related to this PR but it seems that other heather files indent everything inside namespace sdf maybe we should take this opportunity to do the same in this file for consistency with the rest?

Copy link
Copy Markdown
Member

@marcoag marcoag Nov 17, 2021

Choose a reason for hiding this comment

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

Also it seems like inline namespace SDF_VERSION_NAMESPACE { is used for this case.

Copy link
Copy Markdown
Collaborator Author

@jennuine jennuine Nov 18, 2021

Choose a reason for hiding this comment

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

01651ad

@jennuine
Merge branch 'sdf12' into jennuine/preserve_includes
74970ca 
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@jennuine
fixed indent
01651ad 
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
EricCousineau-TRI
EricCousineau-TRI reviewed Nov 18, 2021
View reviewed changes
EricCousineau-TRI
EricCousineau-TRI reviewed Nov 18, 2021
View reviewed changes
@EricCousineau-TRI EricCousineau-TRI mentioned this pull request Dec 2, 2021
Open
@jennuine
test for merge include & interface api
d9ac7e0 
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@jennuine
Merge branch 'sdf12' into jennuine/preserve_includes
3cc1ea6 
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
azeey
azeey approved these changes Dec 9, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Just a couple minor comments, otherwise LGTM!

@jennuine
use ParserConfig
8b5bd67 
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@jennuine jennuine requested a review from azeey December 9, 2021 20:58
@jennuine jennuine merged commit 93c045c into sdf12 Dec 9, 2021
@jennuine jennuine deleted the jennuine/preserve_includes branch December 9, 2021 22:16
@azeey azeey mentioned this pull request Dec 10, 2021
Merged
8 tasks
@osrf-triage
Copy link
Copy Markdown

osrf-triage commented Apr 13, 2022

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marcoag marcoag marcoag left review comments

@azeey azeey azeey approved these changes

@scpeters scpeters Awaiting requested review from scpeters scpeters is a code owner

+1 more reviewer

@EricCousineau-TRI EricCousineau-TRI EricCousineau-TRI left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

🏯 fortress Ignition Fortress 🌱 garden Ignition Garden

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Should have a parser option or command line flag to preserve includes when converting to string

8 participants

@jennuine @codecov-commenter @osrf-triage @azeey @marcoag @EricCousineau-TRI @j-rivero @chapulina