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

API changes for PrintConfig #708

Merged
merged 4 commits into from
Sep 21, 2021
Merged

API changes for PrintConfig #708

merged 4 commits into from
Sep 21, 2021

Conversation

jennuine
Copy link
Collaborator

Summary

Added API changes related to PrintConfig for #689

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
PrintConfig api changes
b5fc6eb 
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@jennuine
removed comment
99562e0 
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@jennuine jennuine requested a review from azeey September 21, 2021 20:01
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Sep 21, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Sep 21, 2021
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 21, 2021
@chapulina chapulina moved this from Inbox to In review in Core development Sep 21, 2021
azeey
azeey reviewed Sep 21, 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.

I think we should also add it to sdf::SDF::PrintValues and sdf::SDF::ToString. The implementation in #689 takes a string for sdf::SDF::PrintValues, but IMO we would be more consistent and have better separation of concerns if it takes sdf::PrintConfig instead. Then the code in ign.cc can parse the command line arguments create the sdf::PrintConfig object and pass it to sdf::SDF::PrintValues().

include/sdf/PrintConfig.hh Outdated Show resolved Hide resolved
include/sdf/Param.hh Show resolved Hide resolved
@jennuine
added PrintConfig to SDF & Param
7810a1b 
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@jennuine
Copy link
Collaborator Author

I think we should also add it to sdf::SDF::PrintValues and sdf::SDF::ToString

Done 7810a1b

azeey
azeey approved these changes Sep 21, 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.

LGTM! Thanks for the quick iteration.

@azeey azeey mentioned this pull request Sep 21, 2021
Merged
8 tasks
@jennuine jennuine merged commit 871e6bc into main Sep 21, 2021
@jennuine jennuine deleted the jennuine/print_config branch September 21, 2021 22:34
Core development automation moved this from In review to Done Sep 21, 2021
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azeey azeey azeey approved these changes

Assignees
No one assigned
Labels
beta Targeting beta release of upcoming collection 🏯 fortress Ignition Fortress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jennuine @azeey @chapulina