obj_print*() gain max argument
#1482
Conversation
|
R/print-str.R
Outdated
| obj_print_data(x, ...) | ||
| obj_print_footer(x, ...) | ||
| obj_print <- function(x, ..., max = NULL) { | ||
| max <- local_max_print(max) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
With local_max_print() and _dispatch we:
- ensure correctness of the
maxarg with a consistent error message - free the implementer from dealing with these details
- correctly handle the case when
maxis set to a value larger thangetOption("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.
There was a problem hiding this comment.
(*) 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_data.default <- function(x, ...) { | ||
| if (length(x) == 0) | ||
| obj_print_data.default <- function(x, ..., max) { | ||
| if (!vec_is(x)) { |
There was a problem hiding this comment.
I don't really see many people using obj_print() if they don't have a vector class, am I missing a use case?
There was a problem hiding this comment.
Should max be passed through if we do keep this?
There was a problem hiding this comment.
obj_print() is called by vctrs for non-vector classes, IIRC vctrs_scalar .
There was a problem hiding this comment.
Hmm but I feel like that is mostly for testing, and we don't export it. Any thoughts on this @lionel- ?
Good catch @DavisVaughan! Co-authored-by: Davis Vaughan <[email protected]>
|
I don't understand why we don't slice in the generic? |
|
@lionel-: IIRC, to slice in the generic, we would have to pass both the sliced vector and the original lengths to the detail methods. Do you prefer this solution? |
|
Don't we pass the whole vector? |
|
Yes, right now we pass the whole vector. Are you proposing the following logic:
? I rather like that. |
|
yup exactly |
| local_options(max.print = max, .frame = frame) | ||
| } | ||
|
|
||
| structure(max, max_print = max_print) |
There was a problem hiding this comment.
Why do we need both max and max_print? Could we take max = getOption("max.print") in the generic instead, to simplify things?
|
|
||
| if (max > max_print) { | ||
| # Avoid truncation in case we're forwarding to print() | ||
| local_options(max.print = max, .frame = frame) |
There was a problem hiding this comment.
-
Is there any downside to unconditionally setting
max.printto simplify things? -
Can you please try and move the
local_options()to the generic and rename this function toas_max_print(). This would simplify things.
This argument is also accepted by
base::print(), but undocumented.Closes #1355.