Feature request: peek function and max lines as optional arguments to ic()

[D3SL]

trafficstars

Being able to seamlessly set ic() to use a different peak function as a one-off instead of needing to call and reset the options functions each and every time before and after would be be the cherry on top.

[lewinfox]

HI @D3SL, thanks for your interest in the project. Could you provide a bit more detail about what you'd like to achieve, and maybe some fake code to illustrate how you might use this? Cheers 😄

Edit: are you thinking something like this?

# Define a custom peek function
my_peek <- function() {
    cat("Look, code!")
}

# Supply it to a specific `ic()` call.
ic(peek_function = my_peek)

[D3SL]

Yes, that's pretty much it. Basically just adding "Options" like icecream.peeking.function as arguments to ic(). That way instead of needing to set options(icecream.peeking.function = head, icecream.max.lines = 5, icecream.foobar=baz) and then reset everything right after you can simply type ic(peeking_function = head, maxlines=5, foobar=baz) for one call and something else entirely for the next.

It's much more idiomatic to treat something that by design will be frequently altered on the fly as an argument than as a global option.

[lewinfox]

Makes sense, thanks. @DominikRafacz what do you think? How does this fit with your work on ic()?

[DominikRafacz]

I definitely think that is a good idea! Actually, adding some of the options as default values for named parameters was my plan all along. I think I can implement that next week.

[lewinfox]

Awesome thanks! Sorry I haven't got to the PR yet, it's been a busy few weeks here. I'll try and read over the existing work this weekend and then we can look to merge once this new feature is tested and working nicely.

[lewinfox]

If we allow these options to be passed to ic() then we should consider allowing the other options to be passed as well. Current opts are detailed in R/icecream-package.R. On quick review I think:

• icecream.enabled: No need to allow this as an option IMO, presumably if you're going to the bother of customising an ic() call you want to actually see the output.
• icecream.prefix: Include
• icecream.include.context: Include
• icecream.peeking.function: Include as discussed
• icecream.max.lines: We'd need to think about how this will interact with people's custom peek functions otherwise we might unexpectedly truncate their output.

@D3SL if you were to supply your own peek function would you expect / want its output to override icecream.max.lines? It seems to me that the answer should be yes, but keen to get your opinion.

[ErdaradunGaztea]

I have a proposal, peeking_function may be expected to take max_lines as one of its (named) arguments. This way the user would be able to customize their function depending on max_lines value (or ignore it altogether).

[DominikRafacz]

@lewinfox I completely agree with your views on including other options as well. Enabling should not be included, all the other options working so far should be included. Max lines is discussable. Actually, I need to confess: I've already implemented most on that on the a new branch on my fork (I have created it before the multiple arguments branch was merged, so it still need to be rebased and adjusted)

Regarding the max lines parameter: I cannot see easier solution than having a separate parameter. There is a few ideas I have in my mind:

• Currently used: having two parameters.
• Suggested by @ErdaradunGaztea : force all peeking functions to take max_lines as a parameter, which could be passed manually then as an optional. This is quite ellegant, but it would require writing wrappers for all the existing functions which could be currently used for peeking: print, regular str, head... I would like all of them to be possible to be controlled in this behavior.
• Another idea: having a dictionary with default values for max line parameters for specific peeking functions. Currently max.lines is used as an argument for ic_print, which internally calls peeking function and then truncates the output. We could have some dictionary that has a suggested value for each function, e.g. for head, max lines is 5. All non-trivial functions take the default value of 1. User can override this behavior by setting the option or providing a parameter.
• We could also extend the previous idea: make a separate function for registering possible peeking functions and adding them to the dictionary. E.g. if somebody is running icecream internally in their package, they can add ic_register(my_custom_peek, max_lines = 5) and then each call to ic with aforementioned peeking function will have max lines of 5.
• Finally, we could join some of those behaviors and make them automatic... E.g. there is a dictionary for suggested peeking functions, while if user-provided function has a parameter called max_lines with default value AND value of option is unset, then the default value is taken...

Are there any simpler solutions? Cause I don't mind implementing those, they just seem too over-complicated for this problem so far... I am open for your thoughts.

[DominikRafacz]

Additionally, there is one thing I would like to discuss. What to do with naming conventions? Currently we have options names following the pattern: icecream.* and words are separated with dots. How should parameters of ic corresponding to those options be named then? Should they have their names identical to option names? I think that would be to wordy, we can prune at least the icecream. part. But should the other part of the name be still separated with dots? That is not compliant with tidyverse naming conventions... On the other hand, having both names with dots as option names and names with underscores as parameter names does not feel right and might be unintuitive for user... Shall we change names of the options then?

[D3SL]

@D3SL if you were to supply your own peek function would you expect / want its output to override icecream.max.lines? It seems to me that the answer should be yes, but keen to get your opinion.

It depends if you mean the default option or the argument. I'm a big proponent of Torvalds' Law: You don't break userspace, ever. Default options and custom functions don't exist in userspace, the final function call does. Arguments given by the user at the function call level should always override everything else cleanly.

For example consider the hypothetical: User Foo defines a custom peek function with max.lines of 8, and then later calls ic(peek.function = mypeek, max.lines = 12).

When they defined the function it was clear they didn't want the default maximum of 5, and when they called ic() they also deliberately typed out max.lines = 12. At each step the user's natural expectation is that their input will be accepted and followed. 8 overrides 5, and then later on 12 overrides both 8 and 5. The user can choose how they want to use the function and also clearly see what effect their inputs have at every step of the process, aiding in debugging.

Basically, think about what any package calling itself "opinionated" would do and then do the opposite.

But should the other part of the name be still separated with dots? That is not compliant with tidyverse naming conventions...

From what I've seen both the RStudioverse and everyone else have made extensive use of underscore separation and dot separation in function parameters, sometimes using both in the same function. The question shouldn't be what RStudio would do, but what's best for the user. max.lines = 5 and max_lines = 5 are both viable options, I think most people find the latter slightly more readable.

[lewinfox]

My thoughts:

I'm against forcing user-supplied functions to have a max_lines parameter, it seems like an extra layer of complexity that we should try and avoid. I agree with @D3SL that whatever you supply in the ic() call should be authoritative, so we can just truncate the output of the user function to whatever the relevant limit is.

On "dots vs underscores" I get the feeling that a lot of R packages use package.option.name for options and argument_name for everything else. Whenever I'm using a reasonably modern R package and I see name.with.dots I think "oh it's a package option of some kind" (or maybe a type conversion like as.character()). Tidyverse maybe-sorta-kinda does this ? (Example from ggplot2). That's why I would go with max.lines in the ic() call, because it links in with the icecream.max.lines option.

This is obviously very subjective - I have no strong opinion. We could always go all-in and support both versions:

ic <- function(..., max.lines = NA, max_lines = NA) {
  if (!is.na(max_lines) && is.na(max.lines)) {
    max.lines <- max_lines
  }
  # ... do stuff with max.lines
}

but I think this is a bit over the top.

So to proceed, shall we say that the output of peek.function can be any length but we'll truncate the output to max.lines if supplied or icecream.max.lines if no value is given for max.lines?

[ErdaradunGaztea]

I'd definitely be against not passing max_lines to peeking function -- because I envisioned that the default peeking function would return a default summary depending on the number of lines available. Say, if max_lines=1, then it would return the length of a list and its names; if max_lines > length(some_list), it would print each element in a new line, giving more detailed summary.

[lewinfox]

That's a good point, but what if someone else comes along with their own peek function that takes a totally different set of arguments? We can't use '...' to pass args because it's already being used. How about 'peek_function_options = list()' as an parameter in 'ic()'? That solves the issue of extensibility without cluttering the ic() call too much.

[DominikRafacz]

I've opened a PR #15 with the feature implemented. It is a baseline feature, though. I have not implemented any changes to the way options worked so far, as it would make the PR to thick. I feel that we have not reached any conclusion and our discussion here is kinda off the main topic, so I suggest to open a new discussion after @lewinfox accepts the current PR

[lewinfox]

Merged to dev, thanks Dominik. @D3SL if you want to grab the develop branch and play around with it please do, let us know if it does what you need. I'll keep this issue open until #14 is merged and sent to CRAN.

[lewinfox]

I've also opened #16 to continue the discussion about how to handle output lines.

[vorpalvorpal]

• icecream.enabled: No need to allow this as an option IMO, presumably if you're going to the bother of customising an ic() call you want to actually see the output.

There is actually a use case for an enabled option. Where there are lots of calls to ic() sprinkled through a codebase, turning on icecream::ic_enable() could print a lot of stuff to the console if there are loops and/or lots of nested functions. Sometimes that might be desirable, but sometimes it might be that you just want a single ic() call to be enabled. Thus you could turn icecream off in the options, but enable it for just a single call.

Eg.

options(icecream.enabled = FALSE)

func1 <- function(x){
  ic(x)
  y <- x +1
  ic(y)
  return(y)
}

func2 <- function(){
  l <- purrr::map(1:1000, \(n) func1(n)
  ic(l, enabled = TRUE)
  return(l)
}

y <- func2()
# > ℹ ic: <env: global> | `l`: 
# > list(foo)

[D3SL]

That's a good point. You may want to spot debug only one thing, such as in a shiny app, and not flood your console with every other ic() call. In such a case the function level enabled argument is more like always_enabled.