Element: update calls to use sdf::Errors output

[marcoag]

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

🎉 New feature

Work towards #820.

Summary

This is an update for the class Element making the calls use sdf::Errors whenever possible to avoid unwanted console outputs.

Test it

Using any method of the Element class with the errors parameter should not print any error.

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.

[codecov]

Codecov Report

Merging #1141 (005f288) into sdf13 (1d1f1d5) will decrease coverage by 0.10%.
The diff coverage is 89.81%.

❗ Current head 005f288 differs from pull request most recent head b9fde35. Consider uploading reports for the commit b9fde35 to get more accurate results

@@            Coverage Diff             @@
##            sdf13    #1141      +/-   ##
==========================================
- Coverage   87.51%   87.42%   -0.10%     
==========================================
  Files         126      126              
  Lines       16248    16314      +66     
==========================================
+ Hits        14220    14262      +42     
- Misses       2028     2052      +24     

Impacted Files	Coverage Δ
src/Element.cc	96.21% <88.23%> (-1.01%)	⬇️
include/sdf/Element.hh	97.67% <95.23%> (-2.33%)	⬇️
python/src/sdf/pyError.cc	80.82% <100.00%> (+0.54%)	⬆️

... and 2 files with indirect coverage changes

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

[azeey]

Looks good, but I would like to see tests that cover some of the most frequently used APIs sugh as Element::Get.

[marcoag]

Added tests for Get and updated the Set existing ones in 4c83cdc.

The rest of functions changed here seem to only fail when the Element is malformed which I find tricky to create without accessing the private attributes of the class. I modified some of the current tests to make sure they don't return errors nor print any. I guess when the switch is done this should be done for all functions.

[azeey]

The rest of functions changed here seem to only fail when the Element is malformed which I find tricky to create without accessing the private attributes of the class. I modified some of the current tests to make sure they don't return errors nor print any. I guess when the switch is done this should be done for all functions.

Yes, some of the functions are very unlikely to emit an error, but we might be able to induce some errors by providing ill formed description files. I'll try to look into this.

[azeey]

The rest of functions changed here seem to only fail when the Element is malformed which I find tricky to create without accessing the private attributes of the class. I modified some of the current tests to make sure they don't return errors nor print any. I guess when the switch is done this should be done for all functions.

Yes, some of the functions are very unlikely to emit an error, but we might be able to induce some errors by providing ill formed description files. I'll try to look into this.

@marcoag I've pushed some changes to azeey/test_element_error_propagation. Namely, I added some tests that check the propagation sdf::Errors in some of the overloads added in this PR.

I was unable to find a way to cause CountNamedElements to emit an error, but in the process of attempting to do that, I realized that by removing SDF_ASSERT calls, we've made it possible for users to end up with a segfault instead of an exception. This is because as opposed to throwing an exception when there is a critical problem, we now insert a fatal error into an sdf::Errors object and continue executing the function. We should instead be checking if a fatal error occurred and returning early. So in the mentioned branch, I have also added a convenience function to check if there are fatal errors after calling a function

sdformat/src/Element.cc

Lines 1243 to 1246 in 6af4be6

	std::size_t lastErrorIndex = _errors.size();
	this->dataPtr->elementDescriptions.push_back(
	parent->GetElementDescription(i)->Clone(_errors));
	if (hasFatalError(_errors, lastErrorIndex))

Would you be able to look through Element.cc and probably also Param.cc and add similar logic to places where returning early is necessary?

[azeey]

Per video call, it would be best to revert all the changes we made code that calls SDF_ASSERT so that exceptions don't cause segfaults. If we remove the console printing in sdf::Exception (

sdformat/src/Exception.cc

Line 52 in 6af4be6

this->Print();

), exceptions could be handled by the end user with no errors printed to the console. I think the original goal of #820 was to remove console messages, not eliminating exceptions, but I think we started removing exceptions as well maybe hoping that it would be fairly straightforward. We should still try to remove exceptions from libsdformat, but we should leave that for another time. /cc @scpeters.

[azeey]

Can you resolve the conflicts?

[azeey]

LGTM! Just a few minor comments

[azeey]

Thanks for iterating! I'll go ahead and merge!