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

Making PrintValues() and ToString() able to not print default elements #575

Merged
merged 24 commits into from
Jun 17, 2021
Merged

Making PrintValues() and ToString() able to not print default elements #575

merged 24 commits into from
Jun 17, 2021

Conversation

marcoag
Copy link
Member

@marcoag marcoag commented May 19, 2021 •
edited
Loading

Related to #287

Summary

Now PrintValues() and ToString() will not output the elements that were added by default during parsing. There's also the option to call them with a boolean flag that will allow the caller to decide weather they want defaults int the output or not.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Code check passed (In source directory, run sh tools/code_check.sh)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review

Note to maintainers: Remember to use Squash-Merge

Marco A. Gutierrez and others added 18 commits April 19, 2021 13:40
adding element default check
d50c89d 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
renaming variables and integration test
233a922 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
checking for error on load
0a3dca6 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@azeey
Merge branch 'sdf9' into marcoag/add_default_check
18c9bb2
fix typo and added test for child defult elements
c1953c8 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
rename set flag since it's only related to parsing
f21a958 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
adding test for default elements in copy and clone
441713b 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
code clean, extra tests for SetExplicitlySetInFile
52cd85f 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
removing whitespaces
051cd48 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@azeey
Add failing test showing that SetExplicitlySetInFile sets the flag on…
23e9447 
… sibling elements

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Merge pull request #1 from azeey/suggestion_add_default_check
a8699ac 
Add failing test showing that SetExplicitlySetInFile incorrectly sets the flag on sibling elements
refactor SetExplicitlySetInFile to avoid sibblings
369ea0d 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
style fixes
bbaef61 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
adding default elements as comments
64f226f 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
test for adding child to default element
4fcfa21 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
rename ptr variables for clarity
761f7bc 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
dont output default elements in tostring and print
5c5e809 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
Merge branch 'sdf9' of https://github.com/osrf/sdformat into marcoag/…
f223cd3 
…add_default_check

Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label May 19, 2021
aaronchongth
aaronchongth requested changes May 20, 2021
View reviewed changes
Copy link
Collaborator

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Just did a quick look through and requested some changes to the API functions. Looks great! Are all the bullet points in #287 addressed at this point, such that it closes it?

include/sdf/Element.hh Outdated Show resolved Hide resolved
include/sdf/Element.hh Outdated Show resolved Hide resolved
include/sdf/Element.hh Outdated Show resolved Hide resolved
include/sdf/Element.hh Outdated Show resolved Hide resolved
include/sdf/Element.hh Outdated Show resolved Hide resolved
fix to function declarations
c978065 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@marcoag
Copy link
Member Author

marcoag commented May 20, 2021

I thought we got to that point, but maybe @azeey should confirm...

@azeey
Copy link
Collaborator

azeey commented May 20, 2021

I thought we got to that point, but maybe @azeey should confirm...

I think #287 only needed #542, but I remember @marcoag mentioning there were still some issues when upconverting SDFormat files. If that's the case, we should probably keep it open until that's resolved.

@marcoag
Copy link
Member Author

marcoag commented May 20, 2021 •
edited
Loading

True that, let me confirm if there's issues upconverting SDFormat with default values first, before closing #287

aaronchongth
aaronchongth approved these changes May 22, 2021
View reviewed changes
Copy link
Collaborator

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

This looks awesome! Thanks for making the changes!

@azeey
Copy link
Collaborator

azeey commented May 24, 2021

Per VC, we should keep the default behavior (when printing via ign sdf -p or using sdf::Element::ToString() of printing values and elements added by default for libsdformat9-libsdformat11. For libsdformat12, we'll change the default behavior to print only user added content.

reverting to maintain default element behavior
376550f 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@marcoag
Copy link
Member Author

marcoag commented May 28, 2021

Now it's a bit of a weird behavior since it will print any default Element but not their default Attributes. Shall we add another flag to print or not attributes in a similar way to Element here?

remove extra blank line
b7b8c2b 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@azeey azeey mentioned this pull request Jun 3, 2021
Closed
@azeey
Copy link
Collaborator

azeey commented Jun 3, 2021 •
edited
Loading

I suggest we have two behaviors: (1) a default behavior that is consistent with what the code did before this PR, (2) a new behavior enabled by a flag that does not print default elements or attributes. So I don't think we need an extra flag.

Marco A. Gutierrez added 2 commits June 10, 2021 09:35
adding flags for default attributes and elements
68b830f 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
removing extra blank lines
daa3bc2 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@marcoag
Copy link
Member Author

marcoag commented Jun 10, 2021

As agreed on our previous VC this has been added:

  • kept behavior consistent with the code before this PR (print/toString default elements and not print/toString default attributes).
  • Added two flags: one to print/toString default elements and another one to print/toString default attributes

marcoag
marcoag commented Jun 10, 2021
View reviewed changes
src/Element.cc Show resolved Hide resolved
azeey
azeey approved these changes Jun 10, 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! Just a few minor comments.

src/Element_TEST.cc Outdated Show resolved Hide resolved
src/Element_TEST.cc Outdated Show resolved Hide resolved
include/sdf/Element.hh Show resolved Hide resolved
src/Element.cc Show resolved Hide resolved
unit test xml correction and clarifications
f90794a 
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@azeey azeey merged commit c60e8f3 into gazebosim:sdf9 Jun 17, 2021
@marcoag marcoag self-assigned this Jun 28, 2021
@marcoag
Copy link
Member Author

marcoag commented Jun 28, 2021

As per VC we agreed to not consider default elements those added when the auto-conversion to the latest sdf version happens.

azeey added a commit to azeey/sdformat that referenced this pull request Jun 28, 2021
@azeey
Revert "Making PrintValues() and ToString() able to not print default…
93bd673 
… elements (gazebosim#575)"

This reverts commit c60e8f3.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@scpeters scpeters mentioned this pull request Jul 1, 2021
Merged
7 tasks
jennuine pushed a commit that referenced this pull request Jul 29, 2021
@azeey @jennuine
Making PrintValues() and ToString() able to not print default elements (
cec38a3 
#575)

Now PrintValues() and ToString() will not output the elements that were added by default during parsing. There's also the option to call them with a boolean flag that will allow the caller to decide weather they want defaults in the output or not.

Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>

Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Jenn Nguyen <jenn@openrobotics.org>
@nkoenig nkoenig mentioned this pull request Mar 4, 2022
Merged
6 tasks
@azeey azeey mentioned this pull request Mar 5, 2022
Open
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

@aaronchongth aaronchongth Awaiting requested review from aaronchongth

Assignees

@marcoag marcoag

Labels
🏰 citadel Ignition Citadel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@marcoag @azeey @aaronchongth