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

use vdiffr conditionally #350

Merged
merged 1 commit into from
Dec 21, 2020
Merged

use vdiffr conditionally #350

merged 1 commit into from
Dec 21, 2020

Conversation

simonpcouch
Copy link
Collaborator

From BDR to the vdiffr maintainers a few weeks ago:

...
> Suggested packages should be used conditionally: see §1.1.3.1 of
> 'Writing R Extensions'.  Some of the requirements of vdiffr are hard to
> install on a platform without X11 such as M1 Macs: see the logs at
> https://www.stats.ox.ac.uk/pub/bdr/M1mac/.
>
> In some cases there are other suggested packages not used conditionally:
> you can check all of them by setting environment variable
> _R_CHECK_DEPENDS_ONLY_=true  -- see
> https://cran.r-project.org/doc/manuals/r-devel/R-ints.html#Tools .
>
> Please correct ASAP and before 2021-01-12 to safely retain the package
> on CRAN.

From the vdiffr maintainers:

Hi everyone,

A simple way to make vdiffr usage conditional is to define your own
`expect_doppelganger()` as follows:

expect_doppelganger <- function(title, fig, path = NULL, ...) {
  testthat::skip_if_not_installed("vdiffr")
  vdiffr::expect_doppelganger(title, fig, path = path, ...)
}

You then call `expect_doppelganger()` without the `vdiffr::` prefix.
See https://github.com/lionel-/ggstance/commit/eac216f6.

Though vdiffr was in our Suggests:, we weren't previously using it conditionally. After these changes,

devtools::check(env_vars = c(NOT_CRAN = "true", `_R_CHECK_DEPENDS_ONLY_` = "true"))

comes back clean for me!

Copy link
Collaborator

@echasnovski echasnovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!
In my other package after much hesitation I decided to go with other wrapper name to avoid possible confusion, but this is also good. If Lionel recommends and uses this, we should be fine too :)

@echasnovski echasnovski merged commit 426bff7 into develop Dec 21, 2020
@simonpcouch simonpcouch deleted the suggests branch December 28, 2020 16:28
@github-actions
Copy link

github-actions bot commented Mar 8, 2021

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants