Skip to content
New issue

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

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

Already on GitHub? # to your account

Put user-facing output under control of 'usethis.quiet' option #424

Merged
merged 9 commits into from
Aug 7, 2018
2 changes: 2 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# usethis 1.3.0.9000

* `getOption("usethis.quiet")` is consulted when printing user-facing messages. Set this option to `TRUE` to suppress output, e.g., to use usethis functions quietly in another package. For example, use `withr::local_options(list(usethis.quiet = TRUE))` in the calling function (#416, #424).

* `proj_path()` is newly exported. Use it to build paths within the active project. Like `proj_get()` and `proj_set()`, it is not aimed at end users, but rather for use in extension packages. End users should use [rprojroot](https://rprojroot.r-lib.org) or its simpler companion, [here](https://here.r-lib.org), to programmatically detect a project and
build paths within it (#415, #425).

Expand Down
7 changes: 6 additions & 1 deletion R/style.R
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
# Helpers --------------------------------------------------------------------

## anticipates usage where the `...` bits make up one line
cat_line <- function(...) {
##
## 'usethis.quiet' is an undocumented option; anticipated usage:
## * eliminate `capture_output()` calls in usethis tests
## * other packages, e.g., devtools can call usethis functions quietly
cat_line <- function(..., quiet = getOption("usethis.quiet", default = FALSE)) {
if (quiet) return(invisible())
cat(..., "\n", sep = "")
}

Expand Down
16 changes: 16 additions & 0 deletions principles.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,19 @@ Uncomfortable fact: `write_union()` uses the active project, if such exists, to
## Home directory

usethis relies on fs for file system operations. The main thing users will notice is the treatment of home directory on Windows. A Windows user's home directory is interpreted as `C:\Users\username` (typical of Unix-oriented tools, like Git and ssh; also matches Python), as opposed to `C:\Users\username\Documents` (R's default on Windows). In order to be consistent everywhere, all paths supplied by the user should be processed with `user_path_prep()`.

## Communicating with the user

User-facing messages are emitted via helpers in `style.R` (see `todo()` and `done()`) and *everything* is eventually routed through `cat_line()`. This is all intentional and should be preserved.

`cat_line()` has a `quiet` argument and `quiet = TRUE` causes it to not produce output. Default value: `quiet = getOption("usethis.quiet", default = FALSE)`.

* Exploited in usethis tests: option is set and unset in `setup.R` and `teardown.R`. Eliminates the need for ubiquitous `capture_output()` calls.
* Other packages can muffle a usethis call via, e.g., `withr::local_options(list(usethis.quiet = TRUE))`.

Implication: don't call `cat_line(..., quiet = FALSE)` lightly, because it breaks the expectation that the option can be used to silence usethis.

You might also notice that usethis communicates with the user via `cat()` instead of `message()`. Why?

* Pragmatic explanation: default styling of `message()` (at least in RStudio) is red, which suggests that something is wrong. We prefer default styling to be more neutral and less alarmist.
* Principled explanation: if one diverts where various streams go, `cat()` follows printed output, whereas `message()` goes to standard error.
Copy link
Member

Choose a reason for hiding this comment

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

FWIW I don't agree with this principal, I think these status messages should be using message() (and stderr) and assuming RStudio / the Windows console did not re-color them I would push harder to make them use message().

If usethis was a normal unix program I would expect these user facing messages to be on stderr as they would be with message().

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you are not wrong. I just wrote this down because Irene had asked (she's using usethis in the package for Tidies of March) and I decide to record the rationale, such as it is. This would not be hard to change, since everything is so centralized.

5 changes: 3 additions & 2 deletions tests/testthat/helper.R
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,11 @@ scoped_temporary_thing <- function(dir = file_temp(pattern = pattern),
withr::defer(proj_set(old_project, force = TRUE, quiet = TRUE), envir = env)
}

withr::local_options(list(usethis.quiet = TRUE))
switch(
thing,
package = capture_output(create_package(dir, rstudio = rstudio, open = FALSE)),
project = capture_output(create_project(dir, rstudio = rstudio, open = FALSE))
package = create_package(dir, rstudio = rstudio, open = FALSE),
project = create_project(dir, rstudio = rstudio, open = FALSE)
)
invisible(dir)
}
Expand Down
2 changes: 2 additions & 0 deletions tests/testthat/setup.R
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,5 @@ if (nzchar(Sys.getenv("CI"))) {
"c485e443")
Sys.setenv(GITHUB_PAT = github_PAT)
}

options(usethis.quiet = TRUE)
1 change: 1 addition & 0 deletions tests/testthat/teardown.R
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
options(usethis.quiet = NULL)
8 changes: 2 additions & 6 deletions tests/testthat/test-create.R
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,7 @@ test_that("create_* works w/ non-existing rel path and absolutizes it", {
withr::with_dir(
path_temp(), {
old_project <- proj$cur
capture_output(
create_package(path_package, rstudio = FALSE, open = FALSE)
)
create_package(path_package, rstudio = FALSE, open = FALSE)
new_proj <- proj_get()
proj_set(old_project, force = TRUE, quiet = TRUE)
}
Expand All @@ -43,9 +41,7 @@ test_that("create_* works w/ non-existing rel path and absolutizes it", {
withr::with_dir(
path_temp(), {
old_project <- proj$cur
capture_output(
create_project(path_project, rstudio = FALSE, open = FALSE)
)
create_project(path_project, rstudio = FALSE, open = FALSE)
new_proj <- proj_get()
proj_set(old_project, force = TRUE, quiet = TRUE)
}
Expand Down
42 changes: 21 additions & 21 deletions tests/testthat/test-edit.R
Original file line number Diff line number Diff line change
Expand Up @@ -19,30 +19,30 @@ test_that("edit_r_XXX() and edit_git_XXX() have default scope", {
## have them or delete them
skip_if_not_ci()

capture_output(expect_error_free(edit_r_profile()))
capture_output(expect_error_free(edit_r_environ()))
capture_output(expect_error_free(edit_r_makevars()))
capture_output(expect_error_free(edit_git_config()))
capture_output(expect_error_free(edit_git_ignore()))
expect_error_free(edit_r_profile())
expect_error_free(edit_r_environ())
expect_error_free(edit_r_makevars())
expect_error_free(edit_git_config())
expect_error_free(edit_git_ignore())
})

test_that("edit_r_XXX('user') ensures the file exists", {
## run these manually if you already have these files or are happy to
## have them or delete them
skip_if_not_ci()

capture_output(edit_r_profile("user"))
edit_r_profile("user")
expect_r_file(".Rprofile")

capture_output(edit_r_environ("user"))
edit_r_environ("user")
expect_r_file(".Renviron")

capture_output(edit_r_makevars("user"))
edit_r_makevars("user")
expect_r_file(".R", "Makevars")

capture_output(edit_rstudio_snippets(type = "R"))
edit_rstudio_snippets(type = "R")
expect_r_file(".R", "snippets", "r.snippets")
capture_output(edit_rstudio_snippets(type = "HTML"))
edit_rstudio_snippets(type = "HTML")
expect_r_file(".R", "snippets", "html.snippets")
})

Expand All @@ -51,57 +51,57 @@ test_that("edit_git_XXX('user') ensures the file exists", {
## have them or delete them
skip_if_not_ci()

capture_output(edit_git_config("user"))
edit_git_config("user")
expect_fs_file(".gitconfig")

capture_output(edit_git_ignore("user"))
edit_git_ignore("user")
expect_fs_file(".gitignore")
cfg <- git2r::config()
expect_match(cfg$global$core.excludesfile, "gitignore")
})

test_that("edit_r_profile() ensures .Rprofile exists in project", {
scoped_temporary_package()
capture_output(edit_r_profile("project"))
edit_r_profile("project")
expect_project_file(".Rprofile")

scoped_temporary_project()
capture_output(edit_r_profile("project"))
edit_r_profile("project")
expect_project_file(".Rprofile")
})

test_that("edit_r_environ() ensures .Renviron exists in project", {
scoped_temporary_package()
capture_output(edit_r_environ("project"))
edit_r_environ("project")
expect_project_file(".Renviron")

scoped_temporary_project()
capture_output(edit_r_environ("project"))
edit_r_environ("project")
expect_project_file(".Renviron")
})

test_that("edit_r_makevars() ensures .R/Makevars exists in package", {
scoped_temporary_package()
capture_output(edit_r_makevars("project"))
edit_r_makevars("project")
expect_project_file(".R", "Makevars")
})

test_that("edit_git_config() ensures git ignore file exists in project", {
scoped_temporary_package()
capture_output(edit_git_config("project"))
edit_git_config("project")
expect_project_file(".git", "config")

scoped_temporary_project()
capture_output(edit_git_config("project"))
edit_git_config("project")
expect_project_file(".git", "config")
})

test_that("edit_git_ignore() ensures .gitignore exists in project", {
scoped_temporary_package()
capture_output(edit_git_ignore("project"))
edit_git_ignore("project")
expect_project_file(".gitignore")

scoped_temporary_project()
capture_output(edit_git_ignore("project"))
edit_git_ignore("project")
expect_project_file(".gitignore")
})
16 changes: 6 additions & 10 deletions tests/testthat/test-helpers.R
Original file line number Diff line number Diff line change
Expand Up @@ -78,21 +78,17 @@ test_that("use_description_field() can address an existing field", {
)

## overwrite existing field
capture_output(
use_description_field(
name = "Version",
value = "1.1.1",
base_path = pkg,
overwrite = TRUE
)
use_description_field(
name = "Version",
value = "1.1.1",
base_path = pkg,
overwrite = TRUE
)
expect_identical(c(Version = "1.1.1"), desc::desc_get("Version", pkg))
})

test_that("use_description_field() can add new field", {
pkg <- scoped_temporary_package()
capture_output(
use_description_field(name = "foo", value = "bar", base_path = pkg)
)
use_description_field(name = "foo", value = "bar", base_path = pkg)
expect_identical(c(foo = "bar"), desc::desc_get("foo", pkg))
})
8 changes: 4 additions & 4 deletions tests/testthat/test-use-badge.R
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,22 @@ context("use_badge")

test_that("use_[cran|bioc]_badge() don't error", {
pkg <- scoped_temporary_package()
expect_error_free(capture_output(use_cran_badge()))
expect_error_free(capture_output(use_bioc_badge()))
expect_error_free(use_cran_badge())
expect_error_free(use_bioc_badge())
})

test_that("use_lifecycle_badge() handles bad and good input", {
pkg <- scoped_temporary_package()
expect_error(use_lifecycle_badge(), "argument \"stage\" is missing")
expect_error(use_lifecycle_badge("eperimental"), "'arg' should be one of ")
expect_error_free(capture_output(use_lifecycle_badge("stable")))
expect_error_free(use_lifecycle_badge("stable"))
})

test_that("use_binder_badge() the github repository works", {
skip_if(getRversion() < 3.2)
skip_if_no_git_config()
scoped_temporary_project()
expect_error_free(capture_output(use_binder_badge()))
expect_error_free(use_binder_badge())
})

test_that("use_badge() does nothing if badge seems to pre-exist", {
Expand Down
2 changes: 1 addition & 1 deletion tests/testthat/test-use-code-of-conduct.R
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ context("use_code_of_conduct")

test_that("use_code_of_conduct() creates promised file", {
scoped_temporary_project()
capture_output(use_code_of_conduct())
use_code_of_conduct()
expect_true(file_exists(proj_path("CODE_OF_CONDUCT.md")))
})
6 changes: 3 additions & 3 deletions tests/testthat/test-use-course.R
Original file line number Diff line number Diff line change
Expand Up @@ -114,19 +114,19 @@ test_that("top_directory() identifies a unique top directory (or not)", {
test_that("tidy_unzip() deals with loose parts, reports unpack destination", {
tmp <- file_temp(ext = ".zip")
file_copy(test_file("yo-loose-regular.zip"), tmp)
capture_output(dest <- tidy_unzip(tmp))
dest <- tidy_unzip(tmp)
loose_regular_files <- path_file(dir_ls(dest, recursive = TRUE))
dir_delete(dest)

tmp <- file_temp(ext = ".zip")
file_copy(test_file("yo-loose-dropbox.zip"), tmp)
capture_output(dest <- tidy_unzip(tmp))
dest <- tidy_unzip(tmp)
loose_dropbox_files <- path_file(dir_ls(dest, recursive = TRUE))
dir_delete(dest)

tmp <- file_temp(ext = ".zip")
file_copy(test_file("yo-not-loose.zip"), tmp)
capture_output(dest <- tidy_unzip(tmp))
dest <- tidy_unzip(tmp)
not_loose_files <- path_file(dir_ls(dest, recursive = TRUE))
dir_delete(dest)

Expand Down
2 changes: 1 addition & 1 deletion tests/testthat/test-use-cran-comments.R
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ test_that("use_cran_comments() requires a package", {

test_that("use_cran_comments() creates and ignores the promised file", {
scoped_temporary_package()
capture_output(use_cran_comments())
use_cran_comments()
expect_true(file_exists(proj_path("cran-comments.md")))
expect_true(is_build_ignored("^cran-comments\\.md$"))
})
12 changes: 6 additions & 6 deletions tests/testthat/test-use-data.R
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ test_that("use_data() stores new, non-internal data", {
pkg <- scoped_temporary_package()
letters2 <- letters
month.abb2 <- month.abb
capture_output(use_data(letters2, month.abb2))
use_data(letters2, month.abb2)
rm(letters2, month.abb2)

load(proj_path("data", "letters2.rda"))
Expand All @@ -21,12 +21,12 @@ test_that("use_data() stores new, non-internal data", {
test_that("use_data() honors `overwrite` for non-internal data", {
pkg <- scoped_temporary_package()
letters2 <- letters
capture_output(use_data(letters2))
use_data(letters2)

expect_error(use_data(letters2), ".*data/letters2.rda.* already exist")

letters2 <- rev(letters)
capture_output(use_data(letters2, overwrite = TRUE))
use_data(letters2, overwrite = TRUE)

load(proj_path("data", "letters2.rda"))
expect_identical(letters2, rev(letters))
Expand All @@ -36,7 +36,7 @@ test_that("use_data() stores new internal data", {
pkg <- scoped_temporary_package()
letters2 <- letters
month.abb2 <- month.abb
capture_output(use_data(letters2, month.abb2, internal = TRUE))
use_data(letters2, month.abb2, internal = TRUE)
rm(letters2, month.abb2)

load(proj_path("R", "sysdata.rda"))
Expand All @@ -47,7 +47,7 @@ test_that("use_data() stores new internal data", {
test_that("use_data() honors `overwrite` for internal data", {
pkg <- scoped_temporary_package()
letters2 <- letters
capture_output(use_data(letters2, internal = TRUE))
use_data(letters2, internal = TRUE)
rm(letters2)

expect_error(
Expand All @@ -56,7 +56,7 @@ test_that("use_data() honors `overwrite` for internal data", {
)

letters2 <- rev(letters)
capture_output(use_data(letters2, internal = TRUE, overwrite = TRUE))
use_data(letters2, internal = TRUE, overwrite = TRUE)

load(proj_path("R", "sysdata.rda"))
expect_identical(letters2, rev(letters))
Expand Down
9 changes: 7 additions & 2 deletions tests/testthat/test-use-dependency.R
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ context("use_dependency")
test_that("we message for new type and are silent for same type", {
scoped_temporary_package()

withr::local_options(list(usethis.quiet = FALSE))

expect_output(
use_dependency("crayon", "Imports"),
"Adding 'crayon' to Imports field"
Expand All @@ -17,6 +19,8 @@ test_that("we message for new type and are silent for same type", {
test_that("we message for version change and are silent for same version", {
scoped_temporary_package()

withr::local_options(list(usethis.quiet = FALSE))

expect_output(use_dependency("crayon", "Imports"))

expect_output(
Expand All @@ -35,9 +39,10 @@ test_that("we message for version change and are silent for same version", {
test_that("use_dependency() upgrades a dependency", {
scoped_temporary_package()

capture_output(use_dependency("usethis", "Suggests"))
use_dependency("usethis", "Suggests")
expect_match(desc::desc_get("Suggests", proj_get()), "usethis")

withr::local_options(list(usethis.quiet = FALSE))
expect_output(use_dependency("usethis", "Imports"), "Removing.*Adding")
expect_match(desc::desc_get("Imports", proj_get()), "usethis")
expect_false(grepl("usethis", desc::desc_get("Suggests", proj_get())))
Expand All @@ -47,7 +52,7 @@ test_that("use_dependency() upgrades a dependency", {
test_that("use_dependency() declines to downgrade a dependency", {
scoped_temporary_package()

capture_output(use_dependency("usethis", "Imports"))
use_dependency("usethis", "Imports")
expect_match(desc::desc_get("Imports", proj_get()), "usethis")

expect_warning(use_dependency("usethis", "Suggests"), "no change")
Expand Down
Loading