paleolimbot commented on issue #565: URL: https://github.com/apache/arrow-nanoarrow/issues/565#issuecomment-2251273106
Thanks for opening! It has been suggested before that we have an `EXPECT_OK()` in a review but I haven't had the time to investigate. I've never really minded `EXPECT_EQ(..., NANOARROW_OK)` but you're right that the failure mode is not great. The flip side of that is that we're a small library that doesn't add features very often (I don't currently spend any of my time wishing that our CI failures had better output because our CI is pretty stable). Perhaps worth considering that other libraries might want to use it too (so we might want `EXPECT_NANOARROW_OK()` instead of `EXPECT_OK()`, or use a matcher since that can be renamed with `using`). One thing that has come up for me in testing is that I would love is to know where a non-`NANOARROW_OK` return came from in debug mode (in Arrow C++ I think this is like the extra error context). Sometimes the `error` is useful but often it is not...some function deep down returns `EINVAL`, `NANOARROW_RETURN_NOT_OK()` does its magic, and all we see at the end is the `EINVAL`. A dedicated macro or matcher would help there (could inspect some global state that holds the traceback on failure). > It'd even be possible to elide the extra macro argument by introducing a C++ wrapper for ArrowError: I'm a skeptical of the magic here...I'd rather have it as an argument to the macro (or matcher) to make it easier for a new or occasional contributor to pick up what's going on. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
