GH-50603: [R] remove assertthat dependency + improve error messages

[olivroy]

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?

• removing assertthat dependency (replaced by rlang's type checkers)
• adding rlang type checkers standalone files.

Maybe assert_is() could be renamed check_class()?

Are these changes tested?

By existing tests.

Are there any user-facing changes?

No, only error messages standardized.

• GitHub Issue: [R] Remove assertthat dependency and improve error messages #50603

[github-actions]

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?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[thisisnic]

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 devtools::test() once your setup is working.

[thisisnic]

*** Unable to retrieve libarrow for version 25.0.0.9000 (windows-x86_64)

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 ARROW_R_DEV environment variable to TRUE will enabled you to use the nightly builds easily: Sys.setenv(ARROW_R_DEV = "true")

[olivroy]

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

[olivroy]

Everything done now! The last failure was caused by an oversight on my end.

[github-actions]

⚠️ GitHub issue #50603 has been automatically assigned in GitHub to PR creator.

[olivroy]

apologies, I still have an old RStudio (without air), but I ran lintr::lint_package() and found no outstanding issues. the full lintr output didn’t show in previous CI runs…

[thisisnic]

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.

[thisisnic]

Would you mind explaining more about this - why copy them not import them, and what do you mean by not licensed?

[olivroy]

This comes from rlang. https://github.com/r-lib/rlang/blob/main/R/standalone-types-check.R I copied 2 functions from there. But the file is marked as unlicensed, which allows you to copy freely.

[olivroy]

Sure, here is the rationale here:

• I added ~ 30 lines of code (since 93 of them are just for added snapshot tests)
• I copied check_logical() and check_character() from rlang to validate arguments are character or logical vectors , so no maintenance is anticipated there ( 70 lines of code)~ removed

The only significant thing thing this PR adds is the migration of assert_is() to rlang type-checkers (it could be renamed to check_class() actually to standardize error messages and is used throughout.

check_null() is a simple function that errors if x is not NULL (used 4 times)

[thisisnic]

Thanks for the explanation and the time and effort put into working on this. After having a proper think, sorry @olivroy, but I'm against merging this as the risks outweigh the benefits:

• many of those rlang type checking functions are flagged as experimental meaning "the author reserves the right to make breaking changes without a deprecation cycle". FWIW, if they were stable, I'd be in favour of switching to using them.
• although this does reduce our number of dependencies, assertthat is old but stable and is a tiny dependency so the impact is minimal
• I'm also opposed to copying in functions from another package, instead of using its exported functions. We have no way of knowing if those functions get updated later other than checking manually, and using existing exported functions means we can rely on the package's tests too

[thisisnic]

@olivroy, I can see you're updating the PR but, to clarify, I mean I'm going to close the PR and issue as we don't need the change in general

[olivroy]

Oh okay, I removed the copied functions after all. You are right that the functions are marked as experimental, but they have been around for a few years (as unexported functions) ... I wonder why they are marked as experimental.

[thisisnic]

Oh okay, I removed the copied functions after all. You are right that the functions are marked as experimental, but they have been around for a few years (as unexported functions) ... I wonder why they are marked as experimental.

Could be worth opening up an issue on the rlang repo - it could be just that these functions were added as experimental and could be stable now but were never marked as such and they'll be happy to update them.