Converter: add sdf::Errors output to API methods #1123

marcoag · 2022-08-31T11:09:51Z

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

🎉 New feature

Work towards #820.

Summary

It moves all error reporting in the Converter class to sdf::Errors. It also includes minimal changes to the parser so the Converter changes can compile and work adequately. A full update to add sdf::Errors support to the parser will follow up on a separate PR so this one is not to big.

After this PR is merged the parser might introduce more errors into the sdf::Errors structure than before. In order to reduce the time this behavior is changed it is recommended to wait until the PR that addresses errors on the parser is ready so it can be merged immediately after this one.

There is one warning that is still printed unless the warnings policy is set to sdf::EnforcementPolicy::ERR.

Test it

Using any method of the Converter class should only print warnings and report all errors through sdf::Errors. If the warnings policy is set to sdf::EnforcementPolicy::ERR warnings should be reported also in the sdf::Errors 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.

scpeters · 2022-09-08T16:32:05Z

src/CMakeLists.txt

+      XmlUtils.cc
+      Utils.cc)


nit: alphabetize

Suggested change

XmlUtils.cc

Utils.cc)

Utils.cc

XmlUtils.cc)

Done in 5612619.

scpeters · 2022-09-08T17:41:18Z

src/Converter.hh

-    /// \param[in] _toVersion Version number in string format
-    /// \param[in] _quiet False to be more verbose
+    /// \param[in] _toVersion Version number in string format.
+    /// \param[out] _errors Vector of errors.


it feels a little weird to put an output variable in the middle of the argument list. I think I favor either making output variables the first argument (like SemanticPose::Resolve) so that subsequent input parameters could be optional.

Also, we sometimes pass Errors by reference and sometimes return from the function. It feels weird to both return bool and write to an Errors vector though.

@azeey what do you think?

it feels a little weird to put an output variable in the middle of the argument list. I think I favor either making output variables the first argument (like SemanticPose::Resolve) so that subsequent input parameters could be optional.

Sounds good to me, will apply the changes on this PR.

Also, we sometimes pass Errors by reference and sometimes return from the function. It feels weird to both return bool and write to an Errors vector though.

This seems a bit more tricky since not all the time an error is added to the structure false is returned, meaning errors.empty() != return false, so I'm not sure if there would be trivial way of checking this without changing behavior.

Just realized, shall we make the ParserConfig also optional?

Since this is an internal function, I recommend against making ParserConfig optional. Whoever is calling should be passing a config.

codecov · 2022-09-14T06:47:03Z

Codecov Report

Merging #1123 (fbd4584) into sdf13 (702dc37) will decrease coverage by 0.32%.
The diff coverage is 53.71%.

@@            Coverage Diff             @@
##            sdf13    #1123      +/-   ##
==========================================
- Coverage   87.53%   87.20%   -0.33%     
==========================================
  Files         124      124              
  Lines       16144    16216      +72     
==========================================
+ Hits        14131    14141      +10     
- Misses       2013     2075      +62

Impacted Files	Coverage Δ
src/parser.cc	`86.94% <47.36%> (-0.51%)`	⬇️
src/Converter.cc	`84.38% <55.47%> (-8.81%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

scpeters

I made some suggestions on how the new APIs in parser.hh should be structured. The Converter.hh APIs are not public, so they are not as critical

include/sdf/parser.hh

azeey · 2022-09-15T21:47:24Z

src/Converter.cc

+      ss << "Unknown convert element["
+         << name
+         << "]";
+      _errors.push_back({ErrorCode::FATAL_ERROR, ss.str()});


Not sure if this is a fatal error.

Definitely not, switched to CONVERSION_ERROR in 5f91d45.

include/sdf/parser.hh

azeey · 2022-10-11T21:50:56Z

include/sdf/parser.hh

+  /// \param[out] _errors Vector of errors.
+  /// \return True on success.
+  SDFORMAT_VISIBLE
+  bool convertFile(const std::string &_filename, const std::string &_version,


Can you add a TODO to deprecate the other overloads?

You mean to deprecate all overloads that we are creating for the error reporting right? where would you say we should put this TODO?

Good question. We should definitely create a ticket for it, but I'm not sure we'll remember to check, so I suggest putting a TODO in at least one of these files (probably parser.hh since it's more frequently updated/looked at)

Added an issue here and the TODO in 5f91d45.

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

This returns Errors instead of bool and makes the SDFPtr the first argument. Signed-off-by: Steve Peters <scpeters@openrobotics.org>

marcoag requested review from azeey and scpeters as code owners August 31, 2022 11:09

scpeters reviewed Sep 8, 2022

View reviewed changes

marcoag changed the base branch from main to sdf13 October 3, 2022 06:12

marcoag force-pushed the marcoag/sdf_error_converter branch from cdf6c6e to 7ab807e Compare October 3, 2022 06:18

Marco A. Gutierrez added 4 commits October 3, 2022 08:30

moving Converter errors to sdf::Error

42c094c

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

grouping in and out parameters

4a1abd8

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

fix to lint error

eb853d8

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

adding win32 buffer error output fix

ecb8fa2

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

marcoag force-pushed the marcoag/sdf_error_converter branch from 7ab807e to ecb8fa2 Compare October 3, 2022 08:57

scpeters reviewed Oct 3, 2022

View reviewed changes

include/sdf/parser.hh Show resolved Hide resolved

include/sdf/parser.hh Outdated Show resolved Hide resolved

include/sdf/parser.hh Outdated Show resolved Hide resolved

azeey approved these changes Oct 11, 2022

View reviewed changes

Marco A. Gutierrez added 2 commits October 13, 2022 09:29

switch FATAL_ERROR to CONVERSION_ERROR

fc741b2

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

adding error change from previous commit and TODO

5f91d45

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

scpeters mentioned this pull request Oct 14, 2022

Different API for convertFile and convertString #1188

Merged

Marco A. Gutierrez and others added 2 commits November 29, 2022 09:04

Merge branch 'sdf13' into marcoag/sdf_error_converter

84e83b1

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

Different API for convertFile and convertString (#1188)

fbd4584

This returns Errors instead of bool and makes the SDFPtr the first argument. Signed-off-by: Steve Peters <scpeters@openrobotics.org>

marcoag merged commit b1a3334 into sdf13 Nov 30, 2022

marcoag deleted the marcoag/sdf_error_converter branch November 30, 2022 07:15

marcoag mentioned this pull request Feb 8, 2023

Go back to SDF_ASSERT instead of FATAL_ERROR #1235

Merged

8 tasks

scpeters mentioned this pull request May 12, 2023

Added new error codes in Python #1280

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converter: add sdf::Errors output to API methods #1123

Converter: add sdf::Errors output to API methods #1123

marcoag commented Aug 31, 2022 •

edited

Loading

scpeters Sep 8, 2022

marcoag Sep 9, 2022

scpeters Sep 8, 2022

marcoag Sep 9, 2022

marcoag Oct 13, 2022

azeey Oct 13, 2022

codecov bot commented Sep 14, 2022 •

edited

Loading

scpeters left a comment

azeey Sep 15, 2022

marcoag Oct 13, 2022 •

edited

Loading

azeey Oct 11, 2022

marcoag Oct 13, 2022

azeey Oct 13, 2022

marcoag Oct 14, 2022

Converter: add sdf::Errors output to API methods #1123

Converter: add sdf::Errors output to API methods #1123

Conversation

marcoag commented Aug 31, 2022 • edited Loading

🎉 New feature

Summary

Test it

Checklist

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

codecov bot commented Sep 14, 2022 • edited Loading

Codecov Report

scpeters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoag Oct 13, 2022 • edited Loading

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

marcoag commented Aug 31, 2022 •

edited

Loading

codecov bot commented Sep 14, 2022 •

edited

Loading

marcoag Oct 13, 2022 •

edited

Loading