Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

noNA conflict of assertthat and terra #110

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

falkmielke
Copy link
Collaborator

@falkmielke falkmielke commented Nov 12, 2024

The assertthat::noNA check used in assertions might be masked by the terra::noNA function.
In those cases, the not-too-instructive error message is thrown:

> noNA(join_mask)
Error: unable to find an inherited method for function `noNA` for signature `x = "logical"`

This happened to me when using modules of the watina package in a non-RStudio context; I think the roxygen::@importFrom requirements prevent the error, for the majority of users. (I cannot tell whether those implicit imports run correctly in debugging contexts.)
However, combined use of terra and watina is quite common, we can not ensure IDE 100%, and the change is minor effort, so I would argue for an explicit call of assertthat::noNA.

The suggested change is nothing more than that: all noNA calls are replaced by assertthat::noNA.

Note: this is supposed to merge into dev_nextrelease, I hope I did it right with the PR.

@falkmielke falkmielke requested a review from florisvdh November 12, 2024 08:05
@florisvdh florisvdh changed the base branch from main to dev_nextrelease November 13, 2024 10:18
@florisvdh
Copy link
Member

It's not strictly needed when using debug mode in R since the package namespace already determines which noNA() applies (background: https://r-pkgs.org/dependencies-mindset-background.html#function-lookup-inside-a-package). But at the same time it's best practice, as recommended by https://r-pkgs.org/dependencies-in-practice.html#sec-dependencies-in-imports. Thanks!

@florisvdh florisvdh merged commit c8bddf7 into dev_nextrelease Nov 13, 2024
4 checks passed
@florisvdh florisvdh deleted the dev_assertthatconflict branch November 13, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants