Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encapsulate std::cerr under a single point of control #656

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

jwnimmer-tri
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri commented Dec 30, 2024

(This isn't necessarily a new feature, but there doesn't seem to be a "cleanup" or "refactor" PR template.)

🎉 New feature

Closes (n/a).

Summary

When integrating this library into a larger application, downstream code doesn't always want to spew onto cerr. To allow customization, we'll refactor to encapsulate the destination inside of a function. Future work can extend this with a mechanism to redirect it (e.g., to send it to spdlog instead of cerr, or to throw an exception instead of returning wrong answers).

Test it

The existing unit tests provide the same level of code-coverage for error printing as previously (e.g., demonstrating that the error printing does not crash).

Checklist

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

When integrating this library into a larger application, downstream
code doesn't always want to spew onto cerr. To allow customization,
we'll refactor to encapsulate the destination inside of a function.
Future work can extent this with a mechanism to redirect it (e.g.,
to send it to spdlog instead of cerr).

Signed-off-by: Jeremy Nimmer <[email protected]>
Copy link

codecov bot commented Dec 30, 2024

Codecov Report

Attention: Patch coverage is 98.76543% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.73%. Comparing base (44e3bba) to head (ef58920).
Report is 6 commits behind head on gz-math8.

Files with missing lines Patch % Lines
src/SphericalCoordinates.cc 96.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           gz-math8     #656      +/-   ##
============================================
+ Coverage     95.71%   95.73%   +0.01%     
============================================
  Files           150      151       +1     
  Lines         10390    10429      +39     
============================================
+ Hits           9945     9984      +39     
  Misses          445      445              

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

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a minor change, otherwise look good to me

src/Error.cc Outdated Show resolved Hide resolved
Signed-off-by: Jeremy Nimmer <[email protected]>
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. FYI, we have recently added spdlog based logging in gz-utils (gazebosim/gz-utils#134). The long term plan is to replace all console logging here in gz-math with the that.

include/gz/math/PiecewiseScalarField3.hh Outdated Show resolved Hide resolved
Signed-off-by: Jeremy Nimmer <[email protected]>
@jwnimmer-tri
Copy link
Contributor Author

I pushed a rename from error_str to errStream.


FYI on spdlog --

I'm less enamored with spdlog than I was years ago. I don't really think it is API and ABI-stable enough to be used a public (header file) dependency in low-level libraries like this one, especially those that try themselves to be ABI-stable. I especially recommend against using spdlog in header files -- I'm going to be working on removing it from Drake header files this year. I realize you can pin it to the distro or something to improve stability, but to me it seems like a bridge too far. My recommendation is to limit the use of spdlog to just the one new Error.cc implementation file which can trampoline the message text into a spdlog logger. (Then, we will still have a single point of control to replace error printing with exception raising.) That's the design I'll be using in Drake.

If you want to work improving the error architecture in gz-math, my advice would actually be to stop printing messages entirely (in a future major release that has API changes). Functions that receive bad input shouldn't log and proceed anyway, they should either throw an exception with the message, or return a std::expected<T, E> wrapper which can denote the return type xor error. Low level libraries like gz-math that hide errors from their callers (by only ever logging them, instead of returning them) are extremely frustrating when we try to use them as building blocks for robust, higher-layer software. In that light, I don't think gz-math really needs any kind of logging whatsoever, so introducing spdlog down here is moving in the wrong direction.

@azeey azeey merged commit ebd0060 into gazebosim:gz-math8 Jan 6, 2025
14 checks passed
@jwnimmer-tri jwnimmer-tri deleted the cerr branch January 6, 2025 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic 🪵 jetty Gazebo Jetty
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants