jonkeane commented on pull request #9689: URL: https://github.com/apache/arrow/pull/9689#issuecomment-810420129
I've taken a look at this and I think this is an overall better (temporary?) solution than wrapping each function call. Absolutely agree that an earlier + clearer error will be good when we can do that. I haven't yet run this locally (yet), but I wanted to confirm my reading of the test suite is correct: All of the operative tests will be skipped on CRAN (when/if the C++ library is unavailable) because of [our wrapping of `test_that`](https://github.com/apache/arrow/blob/master/r/tests/testthat/helper-arrow.R#L58-L63) and [setting `..skip.tests` when arrow isn't available](https://github.com/apache/arrow/blob/master/r/tests/testthat/helper-arrow.R#L19) (tangentially: could we move up our `test_that` wrapper so they are closer, or do we need the `test_that` wrapping lower down for some other reason?). Overall, I think it's good to reduce the code duplication (which makes searching for strings harder) and to me has a slightly easier to see that everything in the arrowExports.cpp will be not included if that one variable is set (compared to now where it might look like we are selectively turning on or off functions as opposed to turning each one of individually). -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
