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

Fix detection of change in dependency #1807

Merged
merged 2 commits into from
Apr 18, 2023

Conversation

DavisVaughan
Copy link
Member

@DavisVaughan DavisVaughan commented Mar 15, 2023

This should fix the failing main branch

In #1789, support was added for the imports field of use_standalone() and internally that used use_package() to add the required packages to Imports. use_package() called use_dependency(), and I think use_dependency() had a small bug in it, which I'll explain below.

The changes caused this test to fail because of snapshot changes

test_that("use_tidy_dependencies() isn't overly informative", {
skip_if_offline("github.com")
create_local_package(fs::path_temp("tidydeps"))
use_package_doc()
withr::local_options(usethis.quiet = FALSE)
expect_snapshot(use_tidy_dependencies())
})

Specifically it was adding this to the snapshots

Execution halted
  "  v Adding '@importFrom lifecycle deprecated' to 'R/tidydeps-package.R'"
  "  v Writing 'NAMESPACE'"
  "  v Writing 'R/import-standalone-purrr.R'"
+ "  * Refer to functions with `rlang::fun()`"

But that is weird, because use_tidy_dependencies() already called use_package("rlang") up above, and its usage of use_standalone("r-lib/rlang", "purrr") should not have caused a "change" in package dependency type that should trigger this how_to_use() line in use_package(), but it obviously does:

usethis/R/package.R

Lines 40 to 43 in a336c67

changed <- use_dependency(package, type, min_version = min_version)
if (changed) {
how_to_use(package, type)
}

For reference the rlang standalone file does not use a versioned version of rlang
https://github.com/r-lib/rlang/blob/9b450274026baa46aa8f4520fdefd87de02e4565/R/standalone-purrr.R#L6

Turns out use_dependency() was actually returning TRUE here by mistake, because there isn't a delta == 0L branch that exits and returns FALSE (i.e. the case of package exists and no change in version). I've tweaked use_dependency() to have a changed variable that keeps track of whether or not something has changed in the DESCRIPTION file (i.e. changed <- TRUE always follows a desc$write() call).

This change results in a snapshot change that I actually think is a bug fix, because now 2 calls to use_package("withr") results in the 2nd call being completely silent


I also sort of question the usage of use_package() in use_standalone() over the quieter use_dependency(), but that is a separate issue, since this one is clearly a bug.

@jennybc
Copy link
Member

jennybc commented Apr 18, 2023

I had already sorted the messaging thing out (b9c4b88), but yes this looks like an improvement in the return value.

@jennybc jennybc merged commit 8499571 into r-lib:main Apr 18, 2023
@DavisVaughan DavisVaughan deleted the fix/test-snapshots branch April 26, 2023 13:26
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