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.

Sign up for GitHub

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

Jump to bottom

PrintConfig option to preserve includes when converting to string #749

Merged
merged 9 commits into from
Dec 9, 2021
Merged

PrintConfig option to preserve includes when converting to string #749

merged 9 commits into from
Dec 9, 2021

Conversation

jennuine
Copy link
Collaborator

🎉 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 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

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
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
/// \return True if <include> tags are preserved.
/// False if they are to be expanded.
public: bool PreserveIncludes() const;

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

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
Member

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
Collaborator Author

Choose a reason for hiding this comment

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

01651ad

@EricCousineau-TRI EricCousineau-TRI mentioned this pull request Dec 2, 2021
Open
azeey
azeey approved these changes Dec 9, 2021
View reviewed changes
Copy link
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 <[email protected]>
@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

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

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

@azeey azeey azeey approved these changes

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

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
6 participants
@jennuine @codecov-commenter @osrf-triage @azeey @marcoag @EricCousineau-TRI