Adding Errors structure to XmlUtils #1296

marcoag · 2023-06-27T10:43:58Z

🎉 New feature

Work towards #820.

Summary

Adds missing sdf::Errors structure as an option to report errors on XmlUitls. Makes sure that no errors are printed and all are reported through the structure when using the functions that include it as parameter.

Test it

Call the functions that report sdf::Errors through a parameter and no errors should be printed but only reported through the sdf structure.

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

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

codecov · 2023-06-27T11:15:01Z

Codecov Report

Attention: Patch coverage is 86.04651% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.57%. Comparing base (edd5d0b) to head (ba8af44).
Report is 16 commits behind head on sdf13.

Files	Patch %	Lines
src/XmlUtils.cc	69.23%	4 Missing ⚠️
src/ParamPassing.cc	92.59%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            sdf13    #1296      +/-   ##
==========================================
- Coverage   87.58%   87.57%   -0.01%     
==========================================
  Files         128      128              
  Lines       17095    17413     +318     
==========================================
+ Hits        14972    15249     +277     
- Misses       2123     2164      +41

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mjcarroll · 2023-07-15T14:14:33Z

include/sdf/Error.hh

@@ -179,6 +179,12 @@ namespace sdf

    /// \brief Generic error during parsing.
    PARSING_ERROR,
+
+    /// \brief File not found error.
+    FILE_NOT_FOUND,


Should this be FILE_NOT_FOUND_ERROR to match the convention of the others?

In the interest of not changing enums as much as possible in stable versions, I'd say we should use the FILE_READ error code.

this enum isn't used in this PR; I've opened #1442 targeting #1295 to implement this suggestion

removed in 2bc407d

src/XmlUtils.cc

mjcarroll · 2023-07-15T14:15:53Z

src/XmlUtils.cc

@@ -58,10 +74,21 @@ tinyxml2::XMLNode *DeepClone(tinyxml2::XMLDocument *_doc,

 /////////////////////////////////////////////////
 std::string ElementToString(const tinyxml2::XMLElement *_elem)
+{


Can you add a test to cover this method?

azeey · 2024-06-26T02:59:56Z

src/Converter.cc

@@ -923,7 +923,8 @@ void Converter::Map(tinyxml2::XMLElement *_elem,
 /////////////////////////////////////////////////
 void Converter::Move(tinyxml2::XMLElement *_elem,
                     tinyxml2::XMLElement *_moveElem,
-                     const bool _copy)
+                     const bool _copy,
+                     sdf::Errors &_errors)


sdf::Errors should be the first argument to be consistent with the rest of the API.

for whatever reason, the internal APIs in Converter.cc all have the sdf::Errors as the last parameter. So the change in this PR is currently consistent with the rest of the private Converter class APIbut not with the public API. If you want the Converter::Move function to be changed, we can, but I wouldn't want to modify more of the Converter class in this PR

I'm good with maintaining consistency within a class. Let's leave it as is.

azeey · 2024-06-26T03:00:51Z

src/XmlUtils.cc

+}
+
+/////////////////////////////////////////////////
+std::string ElementToString(const tinyxml2::XMLElement *_elem,


sdf::Errors should be the first argument to be consistent with the rest of the API.

azeey · 2024-06-26T03:01:33Z

src/XmlUtils.hh

@@ -29,7 +29,7 @@ namespace sdf
  // Inline bracket to help doxygen filtering.
  inline namespace SDF_VERSION_NAMESPACE {

-  /// \brief Perform a deep copy of an XML Node
+  /// \brief Perform a deep copy f an XML Node


Signed-off-by: Steve Peters <[email protected]>

scpeters · 2024-07-02T04:29:59Z

I made a few more changes (see commit messages) and responded to all review comments except the request for tests. I'll come back to this later

* Expect no errors in successful test * Expect XML_ERROR when passing nullptrs Signed-off-by: Steve Peters <[email protected]>

Signed-off-by: Steve Peters <[email protected]>

azeey · 2024-07-19T18:29:48Z

src/Converter.cc

@@ -923,7 +923,8 @@ void Converter::Map(tinyxml2::XMLElement *_elem,
 /////////////////////////////////////////////////
 void Converter::Move(tinyxml2::XMLElement *_elem,
                     tinyxml2::XMLElement *_moveElem,
-                     const bool _copy)
+                     const bool _copy,
+                     sdf::Errors &_errors)


I'm good with maintaining consistency within a class. Let's leave it as is.

Adding Errors structure to XmlUtils

a01e8c4

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

marcoag requested review from azeey and scpeters as code owners June 27, 2023 10:43

github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Jun 27, 2023

fixes to line length

63b96dd

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

mjcarroll reviewed Jul 15, 2023

View reviewed changes

src/XmlUtils.cc Show resolved Hide resolved

mjcarroll reviewed Jul 15, 2023

View reviewed changes

azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023

azeey removed 🎵 harmonic Gazebo Harmonic beta Targeting beta release of upcoming collection labels Aug 26, 2023

scpeters mentioned this pull request Jun 24, 2024

SDF.cc update calls to use sdf::Errors output #1295

Merged

9 tasks

Merge branch 'sdf13' into marcoag/sdf_error_xmlutils

c522430

azeey reviewed Jun 26, 2024

View reviewed changes

scpeters added 7 commits June 26, 2024 14:28

Merge branch 'sdf13' into marcoag/sdf_error_xmlutils

1d06dea

Error.hh: remove unused FILE_NOT_FOUND

2bc407d

Signed-off-by: Steve Peters <[email protected]>

Make Errors first argument in new XmlUtils.hh APIs

c41ec9c

Signed-off-by: Steve Peters <[email protected]>

Converter.hh: add missing parameter doc-string

7bdf9af

Signed-off-by: Steve Peters <[email protected]>

XmlUtils.hh: revert doc-string change

4150eea

Signed-off-by: Steve Peters <[email protected]>

Use DeepClone Errors API in recursive calls

141c19e

Signed-off-by: Steve Peters <[email protected]>

Remove unused ElementToString implementation

76bd200

Signed-off-by: Steve Peters <[email protected]>

scpeters and others added 4 commits July 1, 2024 21:40

Improve DeepClone tests

dd5f0d2

* Expect no errors in successful test * Expect XML_ERROR when passing nullptrs Signed-off-by: Steve Peters <[email protected]>

Add ElementToString tests

ba8af44

Signed-off-by: Steve Peters <[email protected]>

Fix unused variable warnings

78db55e

Signed-off-by: Steve Peters <[email protected]>

Merge branch 'sdf13' into marcoag/sdf_error_xmlutils

a6a5d71

azeey approved these changes Jul 22, 2024

View reviewed changes

scpeters merged commit fe071fa into sdf13 Jul 29, 2024
11 checks passed

scpeters deleted the marcoag/sdf_error_xmlutils branch July 29, 2024 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Errors structure to XmlUtils #1296

Adding Errors structure to XmlUtils #1296

marcoag commented Jun 27, 2023

codecov bot commented Jun 27, 2023 •

edited

Loading

mjcarroll Jul 15, 2023

azeey Aug 23, 2023

scpeters Jun 24, 2024

scpeters Jul 2, 2024

mjcarroll Jul 15, 2023

scpeters Jul 2, 2024

azeey Jun 26, 2024

scpeters Jul 2, 2024

azeey Jul 19, 2024

azeey Jun 26, 2024

scpeters Jul 2, 2024

azeey Jun 26, 2024

scpeters Jul 2, 2024

scpeters commented Jul 2, 2024 •

edited

Loading

azeey Jul 19, 2024

Adding Errors structure to XmlUtils #1296

Adding Errors structure to XmlUtils #1296

Conversation

marcoag commented Jun 27, 2023

🎉 New feature

Summary

Test it

Checklist

codecov bot commented Jun 27, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scpeters commented Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jun 27, 2023 •

edited

Loading

scpeters commented Jul 2, 2024 •

edited

Loading