paleolimbot commented on code in PR #404:
URL: https://github.com/apache/arrow-nanoarrow/pull/404#discussion_r1539353363
##########
src/nanoarrow/utils_test.cc:
##########
@@ -538,3 +538,36 @@ TEST(DecimalTest, DecimalRoundtripBitshiftTest) {
ArrowBufferReset(&buffer);
}
+
+TEST(MaybeTest, ConstructionAndConversion) {
+ // NOTE these will not print nicely in GTest unless we write PrintTo,
Review Comment:
Hmm...right now nanoarrow.hpp is mostly about digging a pit of success for
not leaking memory and is pretty minimal. That said, the failure mode for tests
that use it is currently not all that informative:
```
[ctest]
/Users/dewey/Desktop/rscratch/arrow-nanoarrow/src/nanoarrow/array_test.cc:577:
Failure
[ctest] Value of: nanoarrow::ViewArrayAs<uint32_t>(&array)
[ctest] Expected: has 2 elements where
[ctest] element #0 is equal to 8-byte object <02-00 00-00 01-00 00-00>,
[ctest] element #1 is equal to 8-byte object <03-00 00-00 01-00 00-00>
[ctest] Actual: { 8-byte object <01-00 00-00 01-00 00-00>, 8-byte object
<03-00 00-00 01-00 00-00> }, whose element #0 doesn't match
```
I would perhaps lean towards `#include <iostream>` for the purposes of this
PR, and before the release we can figure out how best to structure the C++
component. Maybe something like `src/nanoarrow/nanoarrow_cpp(hpp?)` could
contain a few files with smaller scopes (e.g., the current `nanoarrow.hpp`),
and `src/nanoarrow/nanoarrow.hpp` would include all of them? That would let
anybody who cared keep a minimal scope but let most people benefit from the
epic that this PR contains?
--
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]
