Skip to content

Code style #663

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

Merged
merged 7 commits into from
Oct 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions R/auto-test.R
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ auto_test <- function(code_path, test_path, reporter = default_reporter(),
TRUE
}
watch(c(code_path, test_path), watcher, hash = hash)

}

#' Watches a package for changes, rerunning tests as appropriate.
Expand All @@ -75,8 +74,10 @@ auto_test <- function(code_path, test_path, reporter = default_reporter(),
#' @seealso [auto_test()] for details on how method works
auto_test_package <- function(pkg = ".", reporter = default_reporter(), hash = TRUE) {
if (!requireNamespace("devtools", quietly = TRUE)) {
stop("devtools required to run auto_test_package(). Please install.",
call. = FALSE)
stop(
"devtools required to run auto_test_package(). Please install.",
call. = FALSE
)
}

pkg <- devtools::as.package(pkg)
Expand Down Expand Up @@ -120,5 +121,4 @@ auto_test_package <- function(pkg = ".", reporter = default_reporter(), hash = T
TRUE
}
watch(c(code_path, test_path), watcher, hash = hash)

}
1 change: 0 additions & 1 deletion R/colour-text.R
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,3 @@ colourise <- function(text, as = c("success", "skip", "warning", "failure", "err
)
testthat_colours[[as]](text)
}

2 changes: 0 additions & 2 deletions R/compare-character.R
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
compare.character <- function(x, y, check.attributes = TRUE, ...,
max_diffs = 5, max_lines = 5,
width = console_width()) {

if (identical(x, y)) {
return(no_difference())
}
Expand Down Expand Up @@ -61,7 +60,6 @@ compare.character <- function(x, y, check.attributes = TRUE, ...,
}

mismatch_character <- function(x, y, diff = !vector_equal(x, y)) {

structure(
list(
i = which(diff),
Expand Down
7 changes: 4 additions & 3 deletions R/compare-numeric.R
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ compare.numeric <- function(x, y,
tolerance = .Machine$double.eps ^ 0.5,
check.attributes = TRUE,
..., max_diffs = 9) {
all_equal <- all.equal(x, y, tolerance = tolerance,
check.attributes = check.attributes, ...)
all_equal <- all.equal(
x, y, tolerance = tolerance,
check.attributes = check.attributes, ...
)
if (isTRUE(all_equal)) {
return(no_difference())
}
Expand Down Expand Up @@ -65,7 +67,6 @@ mismatch_numeric <- function(x, y, diff = !vector_equal(x, y)) {

#' @export
format.mismatch_numeric <- function(x, ..., max_diffs = 9, digits = 3) {

summary <- paste0(x$n_diff, "/", x$n, " mismatches")
if (x$n_diff > 1) {
mu <- format(x$mu_diff, digits = digits, trim = TRUE)
Expand Down
6 changes: 3 additions & 3 deletions R/compare.R
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ print.comparison <- function(x, ...) {

#' @export
#' @rdname compare
compare.default <- function(x, y, ..., max_diffs = 9){
compare.default <- function(x, y, ..., max_diffs = 9) {
same <- all.equal(x, y, ...)
if (length(same) > max_diffs) {
same <- c(same[1:max_diffs], "...")
Expand All @@ -67,8 +67,9 @@ same_type <- function(x, y) identical(typeof(x), typeof(y))
diff_type <- function(x, y) difference(fmt = "Types not compatible: %s is not %s", typeof(x), typeof(y))

same_class <- function(x, y) {
if (!is.object(x) && !is.object(y))
if (!is.object(x) && !is.object(y)) {
return(TRUE)
}
identical(class(x), class(y))
}
diff_class <- function(x, y) {
Expand All @@ -92,4 +93,3 @@ vector_equal <- function(x, y) {
vector_equal_tol <- function(x, y, tolerance = .Machine$double.eps ^ 0.5) {
(is.na(x) & is.na(y)) | (!is.na(x) & !is.na(y) & abs(x - y) < tolerance)
}

6 changes: 3 additions & 3 deletions R/describe.R
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@
#' })

describe <- function(description, code) {
is_invalid_description <- function (description) {
(!is.character(description) || length(description) != 1
|| nchar(description) == 0)
is_invalid_description <- function(description) {
!is.character(description) || length(description) != 1 ||
nchar(description) == 0
}

if (is_invalid_description(description)) {
Expand Down
5 changes: 2 additions & 3 deletions R/evaluate-promise.R
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#' 4
#' })
evaluate_promise <- function(code, print = FALSE) {

warnings <- Stack$new()
handle_warning <- function(condition) {
warnings$push(condition)
Expand All @@ -32,7 +31,8 @@ evaluate_promise <- function(code, print = FALSE) {
temp <- file()
on.exit(close(temp))

result <- withr::with_output_sink(temp,
result <- withr::with_output_sink(
temp,
withCallingHandlers(
withVisible(code),
warning = handle_warning,
Expand All @@ -53,4 +53,3 @@ evaluate_promise <- function(code, print = FALSE) {
messages = get_messages(messages$as_list())
)
}

5 changes: 0 additions & 5 deletions R/expect-equality.R
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ NULL
#' @param ... other values passed to [all.equal()]
expect_equal <- function(object, expected, ..., info = NULL, label = NULL,
expected.label = NULL) {

Copy link
Member

Choose a reason for hiding this comment

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

Ideally styler wouldn't strip this new line. It doesn't matter here, but in general it seems like a bad idea

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we define a rule when to add/keep a newline after an opening brace? Is this just for top-level functions or for blocks in general?

Copy link
Member

Choose a reason for hiding this comment

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

I think I generally prefer a newline if the function def span multiple lines as it more clearly distinguishes the body from the args.

Choose a reason for hiding this comment

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

So should we

  • Add a section to the tidyverse style guide to leave one line blank after a multi-line function declaration and
  • make styler in general preserving line breaks
    ?

Not sure whether for the second bullet, we could also make some blank line formatting optional, but I could not think of anything particularly meaningful. Maybe one can choose to either to not touch blank lines, or to reduce the amount of blank lines to one if there is at least two blank lines next to each other.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how consistently I adhere to this style. There's also the question of wrapping multiline formals - we should probably be consistent with other calls, and if all arguments don't fit on one line, each argument gets it's own line.

act <- quasi_label(enquo(object), label)
exp <- quasi_label(enquo(expected), expected.label)

Expand All @@ -66,7 +65,6 @@ expect_equal <- function(object, expected, ..., info = NULL, label = NULL,
#' @export
#' @rdname equality-expectations
expect_setequal <- function(object, expected) {

act <- quasi_label(enquo(object))
exp <- quasi_label(enquo(expected))

Expand Down Expand Up @@ -103,7 +101,6 @@ expect_equivalent <- function(object, expected, ..., info = NULL, label = NULL,
#' @rdname equality-expectations
expect_identical <- function(object, expected, info = NULL, label = NULL,
expected.label = NULL) {

act <- quasi_label(enquo(object), label)
exp <- quasi_label(enquo(expected), expected.label)

Expand Down Expand Up @@ -131,7 +128,6 @@ expect_identical <- function(object, expected, info = NULL, label = NULL,
#' @rdname equality-expectations
expect_identical <- function(object, expected, info = NULL, label = NULL,
expected.label = NULL) {

act <- quasi_label(enquo(object), label)
exp <- quasi_label(enquo(expected), expected.label)

Expand Down Expand Up @@ -159,7 +155,6 @@ expect_identical <- function(object, expected, info = NULL, label = NULL,
#' @rdname equality-expectations
expect_reference <- function(object, expected, info = NULL, label = NULL,
expected.label = NULL) {

act <- quasi_label(enquo(object), label)
exp <- quasi_label(enquo(expected), expected.label)

Expand Down
3 changes: 0 additions & 3 deletions R/expect-known.R
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ expect_known_output <- function(object, file,
label = NULL,
print = FALSE,
width = 80) {

act <- list()
act$quo <- enquo(object)
act$lab <- label %||% quo_label(act$quo)
Expand Down Expand Up @@ -86,7 +85,6 @@ expect_known_value <- function(object, file,
...,
info = NULL,
label = NULL) {

act <- quasi_label(enquo(object), label)

if (!file.exists(file)) {
Expand Down Expand Up @@ -126,7 +124,6 @@ expect_equal_to_reference <- function(..., update = FALSE) {
#' @param hash Known hash value. Leave empty and you'll be informed what
#' to use in the test output.
expect_known_hash <- function(object, hash = NULL) {

act <- quasi_label(enquo(object))
act_hash <- digest::digest(act$val)
if (!is.null(hash)) {
Expand Down
6 changes: 3 additions & 3 deletions R/expect-named.R
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
expect_named <- function(object, expected, ignore.order = FALSE,
ignore.case = FALSE, info = NULL,
label = NULL) {

act <- quasi_label(enquo(object), label = label)
act$names <- names(act$val)

Expand All @@ -43,7 +42,8 @@ expect_named <- function(object, expected, ignore.order = FALSE,

expect(
identical(act$names, exp_names),
sprintf("Names of %s (%s) don't match %s",
sprintf(
"Names of %s (%s) don't match %s",
act$lab,
paste0("'", act$names, "'", collapse = ", "),
paste0("'", exp_names, "'", collapse = ", ")
Expand All @@ -58,7 +58,7 @@ normalise_names <- function(x, ignore.order = FALSE, ignore.case = FALSE) {
if (is.null(x)) return()

if (ignore.order) x <- sort(x)
if (ignore.case) x <- tolower(x)
if (ignore.case) x <- tolower(x)

x
}
38 changes: 18 additions & 20 deletions R/expect-output.R
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ NULL
#' @export
#' @rdname output-expectations
expect_output <- function(object, regexp = NULL, ..., info = NULL, label = NULL) {

act <- quasi_capture(enquo(object), capture_output, label = label)

if (identical(regexp, NA)) {
Expand Down Expand Up @@ -118,7 +117,6 @@ expect_error <- function(object,
...,
info = NULL,
label = NULL) {

act <- quasi_capture(enquo(object), capture_error, label = label)
msg <- compare_condition(act$cap, act$lab, regexp = regexp, class = class, ...)
expect(is.null(msg), msg, info = info)
Expand All @@ -129,15 +127,16 @@ expect_error <- function(object,
#' @export
#' @rdname output-expectations
expect_condition <- function(object,
regexp = NULL,
class = NULL,
...,
info = NULL,
label = NULL) {

regexp = NULL,
class = NULL,
...,
info = NULL,
label = NULL) {
act <- quasi_capture(enquo(object), capture_condition, label = label)
msg <- compare_condition(act$cap, act$lab, regexp = regexp, class = class, ...,
cond_type = "condition")
msg <- compare_condition(
act$cap, act$lab, regexp = regexp, class = class, ...,
cond_type = "condition"
)
expect(is.null(msg), msg, info = info)

invisible(act$val %||% act$err)
Expand All @@ -148,7 +147,6 @@ expect_condition <- function(object,
#' @rdname output-expectations
expect_message <- function(object, regexp = NULL, ..., all = FALSE,
info = NULL, label = NULL) {

act <- quasi_capture(enquo(object), capture_messages, label = label)
msg <- compare_messages(act$cap, act$lab, regexp = regexp, all = all, ...)
expect(is.null(msg), msg, info = info)
Expand All @@ -160,10 +158,11 @@ expect_message <- function(object, regexp = NULL, ..., all = FALSE,
#' @rdname output-expectations
expect_warning <- function(object, regexp = NULL, ..., all = FALSE,
info = NULL, label = NULL) {

act <- quasi_capture(enquo(object), capture_warnings, label = label)
msg <- compare_messages(act$cap, act$lab, regexp = regexp, all = all, ...,
cond_type = "warnings")
msg <- compare_messages(
act$cap, act$lab, regexp = regexp, all = all, ...,
cond_type = "warnings"
)
expect(is.null(msg), msg, info = info)

invisible(act$val)
Expand Down Expand Up @@ -204,7 +203,7 @@ compare_condition <- function(cond, lab, regexp = NULL, class = NULL, ...,
cond_type,
cond$message,
paste(class(cond), collapse = "/")
))
))
} else {
return()
}
Expand Down Expand Up @@ -253,11 +252,10 @@ compare_condition <- function(cond, lab, regexp = NULL, class = NULL, ...,


compare_messages <- function(messages,
lab,
regexp = NA, ...,
all = FALSE,
cond_type = "messages") {

lab,
regexp = NA, ...,
all = FALSE,
cond_type = "messages") {
bullets <- paste0("* ", messages, collapse = "\n")
# Expecting no messages
if (identical(regexp, NA)) {
Expand Down
3 changes: 2 additions & 1 deletion R/expect-self-test.R
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ show_failure <- function(expr) {
#' @rdname expect_success
#' @param path Path to save failure output
expect_known_failure <- function(path, expr) {
FailureReporter <- R6::R6Class("FailureReporter", inherit = CheckReporter,
FailureReporter <- R6::R6Class("FailureReporter",
inherit = CheckReporter,
public = list(end_reporter = function(...) {})
)

Expand Down
6 changes: 4 additions & 2 deletions R/expectation.R
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,10 @@ as.expectation <- function(x, ...) UseMethod("as.expectation", x)

#' @export
as.expectation.default <- function(x, ..., srcref = NULL) {
stop("Don't know how to convert '", paste(class(x), collapse = "', '"),
"' to expectation.", call. = FALSE)
stop(
"Don't know how to convert '", paste(class(x), collapse = "', '"),
"' to expectation.", call. = FALSE
)
}

#' @export
Expand Down
7 changes: 4 additions & 3 deletions R/expectations-matches.R
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#' }
expect_match <- function(object, regexp, perl = FALSE, fixed = FALSE, ..., all = TRUE,
info = NULL, label = NULL) {

if (fixed) escape <- identity
else escape <- escape_regex

Expand All @@ -38,8 +37,10 @@ expect_match <- function(object, regexp, perl = FALSE, fixed = FALSE, ..., all =
if (length(act$val) == 1) {
values <- paste0("Actual value: \"", escape(encodeString(act$val)), "\"")
} else {
values <- paste0("Actual values:\n",
paste0("* ", escape(encodeString(act$val)), collapse = "\n"))
values <- paste0(
"Actual values:\n",
paste0("* ", escape(encodeString(act$val)), collapse = "\n")
)
}
expect(
if (all) all(matches) else any(matches),
Expand Down
12 changes: 8 additions & 4 deletions R/make-expectation.R
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@
#' make_expectation(df)
make_expectation <- function(x, expectation = "equals") {
obj <- substitute(x)
expectation <- match.arg(expectation,
c("equals", "is_equivalent_to", "is_identical_to"))
expectation <- match.arg(
expectation,
c("equals", "is_equivalent_to", "is_identical_to")
)

dput(substitute(expect_equal(obj, values),
list(obj = obj, expectation = as.name(expectation), values = x)))
dput(substitute(
expect_equal(obj, values),
list(obj = obj, expectation = as.name(expectation), values = x)
))
}
Loading