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

obj_print*() gain max argument #1482

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 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
5 changes: 5 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# vctrs (development version)

* `obj_print()`, `obj_print_header()`, `obj_print_data()` and
`obj_print_footer()` gain `max` argument that controls the maximum number
of items to print. By default, `getOption("max.print")` is consulted
(#1355, @krlmlr).

* `levels.vctrs_vctr()` now returns `NULL` instead of failing (#1186, @krlmlr).

* `vec_assert()` produces a more informative error when `size` is invalid
Expand Down
74 changes: 63 additions & 11 deletions R/print-str.R
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,26 @@
#' @param x A vector
#' @param ... Additional arguments passed on to methods. See [print()] and
#' [str()] for commonly used options
#' @param max The maximum number of items to print, defaults to
#' `getOption("print.max")`.
#' @keywords internal
#' @export
obj_print <- function(x, ...) {
obj_print_header(x, ...)
obj_print_data(x, ...)
obj_print_footer(x, ...)
obj_print <- function(x, ..., max = NULL) {
max <- local_max_print(max)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this whole local function + _dispatch() redirection is a bit complicated. Can you help me understand why we need it? In clock I was able to get print() to work correctly after I already sliced x by passing max all the way through https://github.com/r-lib/clock/blob/0a07f630bc5859e33b34b85380fafe028cfed5eb/R/calendar.R#L21, can we do that here instead?

Then I'd imagine we just pass max through to each of the three helpers, they'd each validate max using some helper like this one https://github.com/r-lib/clock/blob/f3b79db9226fd4af09b1b7175bdfbb4d225386fb/R/utils.R#L263, and then use it as required

If it has something to do with making it easier for subclasses who implement obj_print_data() methods, I'm not sure we should worry about that too much, since if you implement a obj_print_data() method then you'd already have to handle the slicing yourself anyways, so you may as well handle the collection of the max argument too

Copy link
Member Author

Choose a reason for hiding this comment

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

With local_max_print() and _dispatch we:

  • ensure correctness of the max arg with a consistent error message
  • free the implementer from dealing with these details
  • correctly handle the case when max is set to a value larger than getOption("max.print")
  • avoid querying getOption("max.print") in every helper (we need it at least in the data and in the footer)

I rather like this pattern. Slicing really is just one line of code, handling max is like 15 in an extra helper function.

Copy link
Member Author

Choose a reason for hiding this comment

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

(*) if we had a vec_head()

We could also handle the slicing ourselves, but then we'd need to pass a size argument. Also, some vctrs classes might be able to print without slicing explicitly.

obj_print_header_dispatch(x, ..., max = max)
obj_print_data_dispatch(x, ..., max = max)
obj_print_footer_dispatch(x, ..., max = max)
invisible(x)
}

#' @export
#' @rdname obj_print
obj_print_header <- function(x, ...) {
obj_print_header <- function(x, ..., max = NULL) {
max <- local_max_print(max)
return(obj_print_header_dispatch(x, ..., max = max))
UseMethod("obj_print_header")
}
obj_print_header_dispatch <- function(x, ..., max) {
UseMethod("obj_print_header")
}

Expand All @@ -32,32 +40,76 @@ obj_print_header.default <- function(x, ...) {

#' @export
#' @rdname obj_print
obj_print_data <- function(x, ...) {
obj_print_data <- function(x, ..., max) {
krlmlr marked this conversation as resolved.
Show resolved Hide resolved
max <- local_max_print(max)
return(obj_print_data_dispatch(x, ..., max = max))
UseMethod("obj_print_data")
}
obj_print_data_dispatch <- function(x, ..., max) {
UseMethod("obj_print_data")
}

#' @export
obj_print_data.default <- function(x, ...) {
if (length(x) == 0)
obj_print_data.default <- function(x, ..., max) {
if (!vec_is(x)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't really see many people using obj_print() if they don't have a vector class, am I missing a use case?

Copy link
Member

Choose a reason for hiding this comment

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

Should max be passed through if we do keep this?

Copy link
Member Author

Choose a reason for hiding this comment

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

obj_print() is called by vctrs for non-vector classes, IIRC vctrs_scalar .

Copy link
Member

Choose a reason for hiding this comment

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

Hmm but I feel like that is mostly for testing, and we don't export it. Any thoughts on this @lionel- ?

print(x, quote = FALSE)
return(invisible(x))
}

if (vec_size(x) > max) {
x_max <- vec_slice(x, seq_len(max))
} else {
x_max <- x
}

if (vec_size(x_max) == 0) {
return(invisible(x))
}

out <- stats::setNames(format(x), names(x))
out <- stats::setNames(format(x_max), names(x_max))
print(out, quote = FALSE)

invisible(x)
}

#' @export
#' @rdname obj_print
obj_print_footer <- function(x, ...) {
obj_print_footer <- function(x, ..., max = NULL) {
max <- local_max_print(max)
return(obj_print_footer_dispatch(x, ..., max = max))
UseMethod("obj_print_footer")
}
obj_print_footer_dispatch <- function(x, ..., max) {
UseMethod("obj_print_footer")
}

#' @export
obj_print_footer.default <- function(x, ...) {
obj_print_footer.default <- function(x, ..., max = NULL) {
krlmlr marked this conversation as resolved.
Show resolved Hide resolved
if (!vec_is(x)) {
return(invisible(x))
}

delta <- vec_size(x) - max
if (delta > 0) {
cat_line("... and ", big_mark(delta), " more")
krlmlr marked this conversation as resolved.
Show resolved Hide resolved
}
krlmlr marked this conversation as resolved.
Show resolved Hide resolved
invisible(x)
}

local_max_print <- function(max, frame = parent.frame()) {
max_print <- getOption("max.print")
DavisVaughan marked this conversation as resolved.
Show resolved Hide resolved
if (is.null(max)) {
return(max_print)
}

stopifnot(is_integerish(max, 1L, finite = TRUE), max >= 0)
if (max > max_print) {
# Avoid truncation in case we're forwarding to print()
local_options(max.print = max, .frame = frame)
Copy link
Member

Choose a reason for hiding this comment

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

  1. Is there any downside to unconditionally setting max.print to simplify things?

  2. Can you please try and move the local_options() to the generic and rename this function to as_max_print(). This would simplify things.

}
max
}


# str ---------------------------------------------------------------------

Expand Down
11 changes: 7 additions & 4 deletions R/type-list-of.R
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,14 @@ vec_proxy.vctrs_list_of <- function(x, ...) {
# Formatting --------------------------------------------------------------

#' @export
obj_print_data.vctrs_list_of <- function(x, ...) {
if (length(x) == 0)
DavisVaughan marked this conversation as resolved.
Show resolved Hide resolved
return()
obj_print_data.vctrs_list_of <- function(x, ..., max) {
krlmlr marked this conversation as resolved.
Show resolved Hide resolved
out <- vec_data(x)
if (max < length(out)) {
out <- out[seq_len(max)]
}

print(vec_data(x))
print(out)
invisible(x)
}

#' @export
Expand Down
13 changes: 13 additions & 0 deletions R/utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -284,3 +284,16 @@ named <- function(x) {
}
x
}

# Copied from pillar.
#
# Function for the thousand separator, returns "," unless it's used for the
# decimal point, in which case returns "."
big_mark <- function(x) {
# The thousand separator,
# "," unless it's used for the decimal point, in which case "."
mark <- if (identical(getOption("OutDec"), ",")) "." else ","
ret <- formatC(x, big.mark = mark, format = "d", preserve.width = "individual")
ret[is.na(x)] <- "??"
ret
}
11 changes: 7 additions & 4 deletions man/obj_print.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

55 changes: 55 additions & 0 deletions tests/testthat/_snaps/print-str.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,58 @@
$ carb: num 4 4 1 1 2 1 4 2 2 4 ...
@ row.names: chr [1:32] "Mazda RX4" "Mazda RX4 Wag" "Datsun 710" "Hornet 4 Drive" ...

# max argument (#1355)

Code
x <- vctrs::new_vctr(letters)
print(x, max = 5)
Output
<vctrs_vctr[26]>
[1] a b c d e
... and 21 more
Code
print(x, max = 30)
Output
<vctrs_vctr[26]>
[1] a b c d e f g h i j k l m n o p q r s t u v w x y z

# small max.print option (#1355)

Code
x <- vctrs::new_vctr(letters)
print(x)
Output
<vctrs_vctr[26]>
[1] a b c d e
... and 21 more

# large max.print option (#1355)

Code
x <- vctrs::new_vctr(letters)
print(x)
Output
<vctrs_vctr[26]>
[1] a b c d e f g h i j k l m n o p q r s t u v w x y z

# both max argument and max.print option (#1355)

Code
x <- vctrs::new_vctr(letters)
print(x, max = 5)
Output
<vctrs_vctr[26]>
[1] a b c d e
... and 21 more
Code
print(x, max = 20)
Output
<vctrs_vctr[26]>
[1] a b c d e f g h i j k l m n o p q r s t
... and 6 more
Code
print(x, max = 30)
Output
<vctrs_vctr[26]>
[1] a b c d e f g h i j k l m n o p q r s t u v w x y z

11 changes: 11 additions & 0 deletions tests/testthat/_snaps/type-list-of.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,17 @@
[1] 2 3


---

Code
print(list_of(1, 2:3), max = 1)
Output
<list_of<double>[2]>
[[1]]
[1] 1

... and 1 more

---

Code
Expand Down
35 changes: 35 additions & 0 deletions tests/testthat/test-print-str.R
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,38 @@ test_that("show attributes", {

expect_snapshot(obj_str(mtcars))
})

test_that("max argument (#1355)", {
expect_snapshot({
x <- vctrs::new_vctr(letters)
print(x, max = 5)
print(x, max = 30)
})
})

test_that("small max.print option (#1355)", {
local_options(max.print = 5)
expect_snapshot({
x <- vctrs::new_vctr(letters)
print(x)
})
})

test_that("large max.print option (#1355)", {
local_options(max.print = 30)
expect_snapshot({
x <- vctrs::new_vctr(letters)
print(x)
})
})

test_that("both max argument and max.print option (#1355)", {
local_options(max.print = 10)

expect_snapshot({
x <- vctrs::new_vctr(letters)
print(x, max = 5)
print(x, max = 20)
print(x, max = 30)
})
})
1 change: 1 addition & 0 deletions tests/testthat/test-type-list-of.R
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ test_that("is_list_of as expected", {
test_that("print method gives human friendly output", {
skip_on_cran() # Depends on tibble
expect_snapshot(list_of(1, 2:3))
expect_snapshot(print(list_of(1, 2:3), max = 1))
expect_snapshot(tibble::tibble(x = list_of(1, 2:3)))
})

Expand Down