Adding sdf::Error logging to Param #939

marcoag · 2022-04-06T10:44:20Z

🎉 New feature

Work towards #820.

Summary

Adding sdf::Errors &_errors as an argument option to any function that does any logging. The user can now choose to call functions with the sdf::Errors argument to retrieve the error messages or call them without it and get the same old behavior.

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
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.

codecov-commenter · 2022-04-06T11:33:45Z

Codecov Report

Merging #939 (19035ae) into main (ef3a55f) will not change coverage.
The diff coverage is n/a.

❗ Current head 19035ae differs from pull request most recent head 95077d8. Consider uploading reports for the commit 95077d8 to get more accurate results

@@           Coverage Diff           @@
##             main     #939   +/-   ##
=======================================
  Coverage   66.66%   66.66%           
=======================================
  Files           2        2           
  Lines          27       27           
=======================================
  Hits           18       18           
  Misses          9        9

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef3a55f...95077d8. Read the comment docs.

scpeters · 2022-04-09T01:31:48Z

include/sdf/Error.hh

@@ -147,6 +147,9 @@ namespace sdf
    /// \brief Merge include is unspported for the type of entity being
    /// included, or the custom parser does not support merge includes.
    MERGE_INCLUDE_UNSUPPORTED,
+
+    /// \brief Generic error type for parameters.
+    PARAMETER_ERROR


nit: use a trailing comma

include/sdf/Param.hh

azeey

I have some minor comments below, but looks great in general! I think it would be good to add tests to ensure that console messages are not accidentally being printed. You can use sdf::testing::RedirectConsoleStream to capture the console output.

include/sdf/Error.hh

include/sdf/Param.hh

src/Param.cc

scpeters · 2022-04-14T22:30:38Z

src/Param.cc

+      sdferr << errors[i].Message() + "\n";
+    }
+
+    SDF_ASSERT(false, errors.back().Message());


what is the purpose of this assert?

The idea was to keep the same behavior as before (see https://github.com/ignitionrobotics/sdformat/blob/e363110e9c5a3671acb371537a6eaef5617f1d28/src/Param.cc#L106 ). Basically, f there are any errors in the vector use the assert to throw the exception with the last added message as it was done previously.

thanks, I hadn't followed the changes closely enough, but I get it now!

azeey

I believe the only thing left is to add some tests to ensure error messages are not sent to the console.

include/sdf/Param.hh

src/Param.cc

azeey · 2022-05-02T18:01:22Z

test/integration/error_output.cc

@@ -0,0 +1,150 @@
+/*
+ * Copyright 2015 Open Source Robotics Foundation


azeey · 2022-05-02T18:21:58Z

test/integration/error_output.cc

+  sdf::Error error;
+  EXPECT_NO_THROW(sdf::Param param1("key", "not_valid_type", "true", false,
+                                    errors, "description"));
+  error = errors.front();


I recommend adding ASSERT_* calls to check errors contains the number of expected errors. Otherwise, we might get a segfault when accessing the items.

Sure, changed EXPECTs to ASSERT.

azeey · 2022-05-02T20:23:09Z

test/integration/error_output.cc

+                    "description");
+  error = errors.front();
+  errors.erase(errors.begin());
+  ASSERT_EQ(sdf::ErrorCode::PARAMETER_ERROR, error.Code());


I think it would be clearer to index into the errors object than to pop each error and assign to error. Here's an example: https://github.com/ignitionrobotics/sdformat/blob/4fa1be3140b827eaf9a954a5800df04442b23ef9/test/integration/frame.cc#L743-L762

Sure, changed to this format.

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

azeey · 2022-05-04T17:24:44Z

test/integration/error_output.cc

+  sdf::Errors errors;
+  ASSERT_NO_THROW(sdf::Param param1("key", "not_valid_type", "true", false,
+                                    errors, "description"));
+  ASSERT_EQ(errors[0].Code(), sdf::ErrorCode::UNKNOWN_PARAMETER_TYPE);


Sorry, I only meant to use ASSERT to check the size of the errors object. Here is my suggestion #1010

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

…gestions Suggestions for #939

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

marcoag requested review from azeey and scpeters as code owners April 6, 2022 10:44

github-actions bot added the 🌱 garden Ignition Garden label Apr 6, 2022

chapulina added mujoco and removed mujoco labels Apr 6, 2022

scpeters reviewed Apr 9, 2022

View reviewed changes

azeey reviewed Apr 9, 2022

View reviewed changes

marcoag requested a review from azeey April 12, 2022 04:23

azeey mentioned this pull request Apr 13, 2022

Explicitly delimit EOM for logging #820

Open

scpeters reviewed Apr 14, 2022

View reviewed changes

azeey reviewed Apr 21, 2022

View reviewed changes

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

src/Param.cc Outdated Show resolved Hide resolved

azeey mentioned this pull request May 2, 2022

Update focal to use gcc9 gazebo-tooling/action-gz-ci#50

Merged

azeey reviewed May 2, 2022

View reviewed changes

marcoag force-pushed the marcoag/errorlogging_param branch 3 times, most recently from 662bca5 to 95077d8 Compare May 4, 2022 14:01

azeey added a commit that referenced this pull request May 4, 2022

Suggestions for #939

731a562

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

azeey added a commit that referenced this pull request May 4, 2022

Suggestions for #939

9e67f78

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

azeey reviewed May 4, 2022

View reviewed changes

marcoag force-pushed the marcoag/errorlogging_param branch 2 times, most recently from 5554ec3 to 95077d8 Compare May 5, 2022 04:06

adding vector of errors to param functions

4ea168c

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

marcoag force-pushed the marcoag/errorlogging_param branch from 95077d8 to 4ea168c Compare May 5, 2022 12:55

marcoag pushed a commit that referenced this pull request May 5, 2022

Merge pull request #1010 from ignitionrobotics/azeey/errorlogging_sug…

e8ac602

…gestions Suggestions for #939

marcoag force-pushed the marcoag/errorlogging_param branch from ce16a34 to 4ea168c Compare May 5, 2022 14:48

azeey added a commit that referenced this pull request May 5, 2022

Suggestions for #939

9919dc9

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

Suggestions for #939

f9594ac

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

azeey approved these changes May 5, 2022

View reviewed changes

azeey merged commit 3099570 into main May 5, 2022

azeey deleted the marcoag/errorlogging_param branch May 5, 2022 21:09

ahcorde 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

Adding sdf::Error logging to Param #939

Adding sdf::Error logging to Param #939

marcoag commented Apr 6, 2022 •

edited

Loading

codecov-commenter commented Apr 6, 2022 •

edited

Loading

scpeters Apr 9, 2022

azeey left a comment

scpeters Apr 14, 2022

marcoag Apr 18, 2022

scpeters Apr 19, 2022

azeey left a comment

azeey May 2, 2022

azeey May 2, 2022

marcoag May 4, 2022

azeey May 2, 2022

marcoag May 4, 2022

azeey May 4, 2022

		@@ -0,0 +1,150 @@
		/*
		* Copyright 2015 Open Source Robotics Foundation

Adding sdf::Error logging to Param #939

Adding sdf::Error logging to Param #939

Conversation

marcoag commented Apr 6, 2022 • edited Loading

🎉 New feature

Summary

Checklist

codecov-commenter commented Apr 6, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

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

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

marcoag commented Apr 6, 2022 •

edited

Loading

codecov-commenter commented Apr 6, 2022 •

edited

Loading