The value returned by print should be ignored

[hadley]

Because this matches base R behaviour:

print.foo <- function(x) NULL
structure(1, class = "foo")

[lionel-]

I like the simplification and the better alignment with the R REPL. But is this likely to cause breaking changes in downstream deps?

[hadley]

I think it's pretty unlikely that this change will affect much existing code (since this is rather esoteric behaviour), but Christoph will have a better sense since he's running this changes through various additional knitr checks.

[cderv]

FWIW this PR (or something in main that has been merged in) is breaking knitr examples, so I need to investigate what. I'll make a reprex of this.

[cderv]

```{r, include = FALSE}
library(ggplot2)
p <- ggplot(diamonds) + geom_point(aes(x = carat, y = price))
ggsave('plot.png', p)
```

```{r}
knitr::include_graphics('plot.png')
```

Now do this

> knitr::knit("test.Rmd")
> xfun::file_string("test.md")



``` r
knitr::include_graphics('plot.png')
```

No more output included.

This could be side effect of this change with what knitr is doing with the value output handler

# import output handlers from evaluate
default_handlers = evaluate:::default_output_handler
# change the value handler in evaluate default handlers
knit_handlers = function(fun, options) {
  if (!is.function(fun)) fun = function(x, ...) {
    res = withVisible(knit_print(x, ...))
    # indicate the htmlwidget result with a special class so we can attach
    # the figure caption to it later in sew.knit_asis
    if (inherits(x, 'htmlwidget'))
      class(res$value) = c(class(res$value), 'knit_asis_htmlwidget')
    if (res$visible) res$value else invisible(res$value)
  }
  if (length(formals(fun)) < 2)
    stop("the chunk option 'render' must be a function of the form ",
         "function(x, options) or function(x, ...)")
  merge_list(default_handlers, list(
    value = function(x, visible) {
      if (visible) fun(x, options = options)
    },
    calling_handlers = options$calling.handlers
  ))
}

at : https://github.com/yihui/knitr/blob/4a02625e0dce1b88f1c90396790e1eb40401a779/R/utils.R#L860-L882

I did not try to see the link between this change in PR and that part. Just sharing the problem.

[hadley]

Ah yes, I didn't think about the consequences of this change for knitr. I'll contemplate it some more.

[hadley]

@cderv does this work with current knitr + evaluate?

knit_handlers = function(fun, options) {
  if (!is.function(fun)) fun = function(x, ...) {
    res = withVisible(knit_print(x, ...))
    # indicate the htmlwidget result with a special class so we can attach
    # the figure caption to it later in sew.knit_asis
    if (inherits(x, 'htmlwidget'))
      class(res$value) = c(class(res$value), 'knit_asis_htmlwidget')    
    if (res$visible) print(res$value)
  }
  if (length(formals(fun)) < 2)
    stop("the chunk option 'render' must be a function of the form ",
         "function(x, options) or function(x, ...)")
  merge_list(default_handlers, list(
    value = function(x, visible) {
      if (visible) fun(x, options = options)
    },
    calling_handlers = options$calling.handlers
  ))
}

If so, we could update knitr and then we can make this change to evaluate in a couple of years.

[cderv]

No it won't work. Same example but this time the problem is different.

When using include_graphics(), res when inside value from knit_handler will be this:

> dput(res)
list(value = structure("plot.png", class = c("knit_image_paths", 
"knit_asis")), visible = TRUE)
```

when printing, it will be 
````r
> print(res$value)
[1] "plot.png"
attr(,"class")
[1] "knit_image_paths" "knit_asis"   

which will be captured as a single character string in the knitr processing the output res to process as the full printed value

> str(res)
List of 2
 $ :List of 1
  ..$ src: chr "knitr::include_graphics('plot.png')"
  ..- attr(*, "class")= chr "source"
 $ : chr "[1] \"plot.png\"\nattr(,\"class\")\n[1] \"knit_image_paths\" \"knit_asis\"       \n"

So it is not what is expected at all. With new evaluate dev, it will have also the new class for print method

> str(res)
List of 2
 $ :List of 1
  ..$ src: chr "knitr::include_graphics('plot.png')"
  ..- attr(*, "class")= chr "source"
 $ : chr "[1] \"plot.png\"\nattr(,\"class\")\n[1] \"knit_image_paths\" \"knit_asis\"       \n"
 - attr(*, "class")= chr [1:2] "evaluate_evaluation" "list"

it should be

> str(res)
List of 2
 $ :List of 1
  ..$ src: chr "knitr::include_graphics('plot.png')"
  ..- attr(*, "class")= chr "source"
 $ : 'knit_image_paths' chr "plot.png"

which is required for the sew() and choosing the right method.

[hadley]

Hmmmm, I see. In case I'm going to close this issue and totally rethink it (and consider if I really believe it's worth fixing).