Make throwOrPrintErrors a member of sdf::Error #1220

marcoag · 2023-01-13T18:51:12Z

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

🎉 New feature

Related to #1141.

Summary

Because of the request here and as per IM, moving throwOrPrintErrors into Error.cc so now an Error has a method to be thrown or printed and this can be used in current SDFormat header files.

Test it

Throw any Error object using the new method.

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-01-13T19:13:37Z

Codecov Report

Merging #1220 (f6f1715) into sdf13 (863fe57) will increase coverage by 0.00%.
The diff coverage is 83.33%.

@@           Coverage Diff           @@
##            sdf13    #1220   +/-   ##
=======================================
  Coverage   87.20%   87.20%           
=======================================
  Files         124      124           
  Lines       16229    16232    +3     
=======================================
+ Hits        14152    14155    +3     
  Misses       2077     2077

Impacted Files	Coverage Δ
src/Error.cc	`98.48% <80.00%> (-1.52%)`	⬇️
src/Utils.cc	`87.64% <100.00%> (+0.41%)`	⬆️

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

azeey

quick nit: Since the title of the PR becomes part of the changelog, mind changing the tense to match the rest of the changelog? We commonly use imperative ("Move throwOrPrintErrors...") or past tense ("Moved throwOrPrintErrors...")

azeey · 2023-01-13T19:07:38Z

include/sdf/Error.hh

@@ -247,6 +247,10 @@ namespace sdf
    /// \sa Element::SetXmlPath
    public: void SetXmlPath(const std::string &_xmlPath);

+    /// \brief It will print the error to sdferr or throw it using
+    /// SDF_ASSERT depending on its ErrorCode.
+    public: void throwOrPrintError();


If it's a member function, the first letter should be uppercase.

Done in f6f1715.

azeey · 2023-01-13T19:08:47Z

include/sdf/Error.hh

@@ -247,6 +247,10 @@ namespace sdf
    /// \sa Element::SetXmlPath
    public: void SetXmlPath(const std::string &_xmlPath);

+    /// \brief It will print the error to sdferr or throw it using
+    /// SDF_ASSERT depending on its ErrorCode.
+    public: void throwOrPrintError();


Since it's not modifying anything, it should be a const function

True, done in 6483e3a.

azeey · 2023-01-13T19:09:22Z

src/Utils.cc

@@ -167,18 +167,11 @@ void enforceConfigurablePolicyCondition(
 }

 /////////////////////////////////////////////////
-void throwOrPrintErrors(const sdf::Errors& _errors)
+void throwOrPrintErrors(sdf::Errors& _errors)


Shouldn't have to make this change if the member function is const.

Changed back in 6483e3a.

azeey · 2023-01-13T19:15:52Z

src/Error.cc

@@ -118,6 +120,19 @@ void Error::SetXmlPath(const std::string &_xmlPath)
  this->dataPtr->xmlPath = _xmlPath;
 }

+/////////////////////////////////////////////////
+void Error::throwOrPrintError()


I think it would be a good idea for this to take an ostream object as input so it can be used with sdfwarn and others.

sdferr and sdfwarn seem to be ConsoleStream shall it take that one as input instead of the ostream?

Added a ConsoleStream for now in 6483e3a. Let me know if we should do something different.

Yeah, I thought ConsoleStream inherited from ostream, but that's not the case, so what you have is good.

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

azeey · 2023-01-17T21:59:01Z

src/Error.cc

@@ -118,6 +120,19 @@ void Error::SetXmlPath(const std::string &_xmlPath)
  this->dataPtr->xmlPath = _xmlPath;
 }

+/////////////////////////////////////////////////
+void Error::throwOrPrintError()


Yeah, I thought ConsoleStream inherited from ostream, but that's not the case, so what you have is good.

#1220 added sdf::Error::ThrowOrPrintError, but after further thought, I think it would be better if we made it a free function inside the internal namespace as this allows us to remove the function without a deprecation cycle. Since the future goal of libsdformat is to remove exceptions, it is likely that we'd want to remove ThrowOrPrintError in the not too distance future. Signed-off-by: Addisu Z. Taddese <[email protected]>

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

#1220 added sdf::Error::ThrowOrPrintError, but after further thought, I think it would be better if we made it a free function inside the internal namespace as this allows us to remove the function without a deprecation cycle. Since the future goal of libsdformat is to remove exceptions, it is likely that we'd want to remove ThrowOrPrintError in the not too distance future. Signed-off-by: Addisu Z. Taddese <[email protected]>

moving throwOrPrintErrors into Error.cc

3b2d49d

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

marcoag requested review from azeey and scpeters as code owners January 13, 2023 18:51

github-actions bot added the 🌱 garden Ignition Garden label Jan 13, 2023

marcoag mentioned this pull request Jan 13, 2023

Element: update calls to use sdf::Errors output #1141

Merged

9 tasks

azeey requested changes Jan 13, 2023

View reviewed changes

make throwOrPrintError const and take a Console

6483e3a

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

marcoag changed the title ~~moving throwOrPrintErrors into Error.cc~~ Move throwOrPrintErrors into Error.cc Jan 16, 2023

marcoag changed the title ~~Move throwOrPrintErrors into Error.cc~~ Make throwOrPrintErrors a member of sdf::Error Jan 16, 2023

make uppercase as it's a member function

f6f1715

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

azeey approved these changes Jan 17, 2023

View reviewed changes

marcoag merged commit de8b30d into sdf13 Jan 17, 2023

marcoag deleted the marcoag/move_throwOrPrintError branch January 17, 2023 22:02

azeey mentioned this pull request Jan 18, 2023

Make ThrowOrPrintError a free internal function #1221

Merged

8 tasks

mjcarroll pushed a commit that referenced this pull request Feb 9, 2023

Make throwOrPrintErrors a member of sdf::Error (#1220)

013925a

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make throwOrPrintErrors a member of sdf::Error #1220

Make throwOrPrintErrors a member of sdf::Error #1220

marcoag commented Jan 13, 2023

codecov bot commented Jan 13, 2023 •

edited

Loading

azeey left a comment

azeey Jan 13, 2023

marcoag Jan 16, 2023

azeey Jan 13, 2023

marcoag Jan 16, 2023

azeey Jan 13, 2023

marcoag Jan 16, 2023

azeey Jan 13, 2023

marcoag Jan 15, 2023

marcoag Jan 16, 2023

azeey Jan 17, 2023

azeey Jan 17, 2023

Make throwOrPrintErrors a member of sdf::Error #1220

Make throwOrPrintErrors a member of sdf::Error #1220

Conversation

marcoag commented Jan 13, 2023

🎉 New feature

Summary

Test it

Checklist

codecov bot commented Jan 13, 2023 • edited Loading

Codecov Report

azeey left a 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

codecov bot commented Jan 13, 2023 •

edited

Loading