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

marcoag · 2021-05-19T11:02:25Z

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

Signed-off-by: Marco A. Gutierrez <[email protected]>

… sibling elements Signed-off-by: Addisu Z. Taddese <[email protected]>

Add failing test showing that SetExplicitlySetInFile incorrectly sets the flag on sibling elements

Signed-off-by: Marco A. Gutierrez <[email protected]>

…add_default_check Signed-off-by: Marco A. Gutierrez <[email protected]>

aaronchongth

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

Signed-off-by: Marco A. Gutierrez <[email protected]>

marcoag · 2021-05-20T14:26:43Z

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

azeey · 2021-05-20T14:38:47Z

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 · 2021-05-20T14:52:27Z

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

aaronchongth

This looks awesome! Thanks for making the changes!

azeey · 2021-05-24T14:43:42Z

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.

Signed-off-by: Marco A. Gutierrez <[email protected]>

marcoag · 2021-05-28T06:41:14Z

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?

Signed-off-by: Marco A. Gutierrez <[email protected]>

azeey · 2021-06-03T21:31:57Z

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.

Signed-off-by: Marco A. Gutierrez <[email protected]>

marcoag · 2021-06-10T09:44:57Z

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

src/Element.cc

azeey

LGTM! Just a few minor comments.

src/Element_TEST.cc

include/sdf/Element.hh

src/Element.cc

Signed-off-by: Marco A. Gutierrez <[email protected]>

marcoag · 2021-06-28T07:53:17Z

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

… elements (gazebosim#575)" This reverts commit c60e8f3. Signed-off-by: Addisu Z. Taddese <[email protected]>

#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 <[email protected]> Co-authored-by: Addisu Z. Taddese <[email protected]> Signed-off-by: Jenn Nguyen <[email protected]>

Marco A. Gutierrez and others added 18 commits April 19, 2021 13:40

adding element default check

d50c89d

Signed-off-by: Marco A. Gutierrez <[email protected]>

renaming variables and integration test

233a922

Signed-off-by: Marco A. Gutierrez <[email protected]>

checking for error on load

0a3dca6

Signed-off-by: Marco A. Gutierrez <[email protected]>

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 <[email protected]>

rename set flag since it's only related to parsing

f21a958

Signed-off-by: Marco A. Gutierrez <[email protected]>

adding test for default elements in copy and clone

441713b

Signed-off-by: Marco A. Gutierrez <[email protected]>

code clean, extra tests for SetExplicitlySetInFile

52cd85f

Signed-off-by: Marco A. Gutierrez <[email protected]>

removing whitespaces

051cd48

Signed-off-by: Marco A. Gutierrez <[email protected]>

Add failing test showing that SetExplicitlySetInFile sets the flag on…

23e9447

… sibling elements Signed-off-by: Addisu Z. Taddese <[email protected]>

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 <[email protected]>

style fixes

bbaef61

Signed-off-by: Marco A. Gutierrez <[email protected]>

adding default elements as comments

64f226f

Signed-off-by: Marco A. Gutierrez <[email protected]>

test for adding child to default element

4fcfa21

Signed-off-by: Marco A. Gutierrez <[email protected]>

rename ptr variables for clarity

761f7bc

Signed-off-by: Marco A. Gutierrez <[email protected]>

dont output default elements in tostring and print

5c5e809

Signed-off-by: Marco A. Gutierrez <[email protected]>

Merge branch 'sdf9' of https://github.com/osrf/sdformat into marcoag/…

f223cd3

…add_default_check Signed-off-by: Marco A. Gutierrez <[email protected]>

github-actions bot added the 🏰 citadel Ignition Citadel label May 19, 2021

marcoag requested review from azeey and aaronchongth May 19, 2021 11:02

aaronchongth requested changes May 20, 2021

View reviewed changes

fix to function declarations

c978065

Signed-off-by: Marco A. Gutierrez <[email protected]>

aaronchongth approved these changes May 22, 2021

View reviewed changes

reverting to maintain default element behavior

376550f

Signed-off-by: Marco A. Gutierrez <[email protected]>

remove extra blank line

b7b8c2b

Signed-off-by: Marco A. Gutierrez <[email protected]>

azeey mentioned this pull request Jun 3, 2021

Permit detecting an element / attribute was explicitly set (user) or implicitly set (conversion / default value) #287

Closed

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 <[email protected]>

removing extra blank lines

daa3bc2

Signed-off-by: Marco A. Gutierrez <[email protected]>

marcoag requested a review from aaronchongth June 10, 2021 09:46

marcoag commented Jun 10, 2021

View reviewed changes

src/Element.cc Show resolved Hide resolved

azeey approved these changes Jun 10, 2021

View reviewed changes

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 <[email protected]>

azeey merged commit c60e8f3 into gazebosim:sdf9 Jun 17, 2021

marcoag self-assigned this Jun 28, 2021

azeey added a commit to azeey/sdformat that referenced this pull request Jun 28, 2021

Revert "Making PrintValues() and ToString() able to not print default…

93bd673

… elements (gazebosim#575)" This reverts commit c60e8f3. Signed-off-by: Addisu Z. Taddese <[email protected]>

scpeters mentioned this pull request Jul 1, 2021

🎈 11.2.2 #616

Merged

7 tasks

nkoenig mentioned this pull request Mar 4, 2022

Root to element #842

Merged

6 tasks

azeey mentioned this pull request Mar 5, 2022

ToString doesn't skip default elements and attributes on sdf::ElementPtr returned from ToElement #867

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

marcoag commented May 19, 2021 •

edited

Loading

aaronchongth left a comment

marcoag commented May 20, 2021

azeey commented May 20, 2021

marcoag commented May 20, 2021 •

edited

Loading

aaronchongth left a comment

azeey commented May 24, 2021

marcoag commented May 28, 2021

azeey commented Jun 3, 2021 •

edited

Loading

marcoag commented Jun 10, 2021

azeey left a comment

marcoag commented Jun 28, 2021

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

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

Conversation

marcoag commented May 19, 2021 • edited Loading

Summary

Checklist

aaronchongth left a comment

Choose a reason for hiding this comment

marcoag commented May 20, 2021

azeey commented May 20, 2021

marcoag commented May 20, 2021 • edited Loading

aaronchongth left a comment

Choose a reason for hiding this comment

azeey commented May 24, 2021

marcoag commented May 28, 2021

azeey commented Jun 3, 2021 • edited Loading

marcoag commented Jun 10, 2021

azeey left a comment

Choose a reason for hiding this comment

marcoag commented Jun 28, 2021

marcoag commented May 19, 2021 •

edited

Loading

marcoag commented May 20, 2021 •

edited

Loading

azeey commented Jun 3, 2021 •

edited

Loading