GH-50603: [R] remove assertthat dependency + improve error messages - #50525
GH-50603: [R] remove assertthat dependency + improve error messages#50525olivroy wants to merge 28 commits into
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
This comment was marked as resolved.
This comment was marked as resolved.
|
Thanks for the PR @olivroy! Can you expand more about trying to get the dev version working locally - what have you tried and what errors are you getting? It'll be easier if we help you with that first, as building iteratively on CI burns through a lot of machine time and is a really frustrating process as you wait for the builds to complete which takes much longer than just running |
…porting the functions used.
This comment was marked as resolved.
This comment was marked as resolved.
Thanks, that's helpful! It looks like there's an issue with our nightly C++ builds, so actually while we're fixing that then we can probably only easily build on CI so that's fine for now. This is super useful to know though, will have a look at getting it fixed. Once it is fixed, I think that setting the |
|
Oh, this is great! Thanks for investigating and finding a fix! This will be useful in the future. Hopefully, I got everything corrected this time |
… add check_null()
…and `check_character()` + remove assertthat dependency.
|
I guess this would be ready for review. I am now able to build arrow locally!
Edit: lints corrected now! |
|
Could you open a new issue for this? |
|
Everything done now! The last failure was caused by an oversight on my end. |
|
|
|
apologies, I still have an old RStudio (without air), but I ran |
thisisnic
left a comment
There was a problem hiding this comment.
Thanks for this PR @olivroy
One thing to note - with changes like this, it's usually better to open an issue first to discuss with maintainers to see folks agree with the need for the change. No big deal here, just could save time/effort if we don't go ahead with it in the end.
Can you explain more about the reasoning for these changes here? This PR removes a dependency, but overall adds ~200 lines of code to the codebase, which is more code for us to maintain, so I'm not sure it's an improvement, but could be convinced otherwise.
| # copied from https://github.com/r-lib/rlang/blob/main/R/standalone-types-check.R | ||
| # they are not licensed. |
There was a problem hiding this comment.
Would you mind explaining more about this - why copy them not import them, and what do you mean by not licensed?
Rationale for this change
To get rid of assertthat dependency, and use more rlang in error handling. fix #50603
What changes are included in this PR?
Maybe
assert_is()could be renamedcheck_class()?Are these changes tested?
By existing tests.
Are there any user-facing changes?
No, only error messages standardized.