Add support to html_vignette with as_is: true in pkgdown

[GegznaV]

This pull request addresses the parameter clash issue when the output format is html_vignette and as_is: true (see #955) by introducing a new YAML parameter set_null_theme which should be set to false in this case. E.g.:

output:
  bookdown::html_document2:
    base_format: rmarkdown::html_vignette
pkgdown:
  as_is: true
  set_null_theme: false

You may install pkgdown with my changes

remotes::install_github("forked-packages/pkgdown", ref = "fix-955")

and try code:

pkgdown::build_article("test-pkgdown-ok")   # Successful built of vignette is expected
pkgdown::build_article("test-pkgdown-fail") # Failure is expected

in package test.pkgdown.zip to make sure that the changes do as what they are expected to do.

Solves #955.

[GegznaV]

I see that the test coverage test fails with error:

But I cannot imagine how my changes could be related to the wrong version of covr on GH Actions:
package ‘covr’ was installed before R 4.0.0: please re-install it
They are not related.

[Robinlovelace]

I'm not sure how to watch the progress of this commit but I'm commenting here so I get alerted to revert this saferactive/trafficalmr#14

[Robinlovelace]

Just a thought: it may be worth documenting how to use bookdown tags in vignettes.

[GegznaV]

Just a thought: it may be worth documenting how to use bookdown tags in vignettes.

@Robinlovelace, do you mean (1) this:

pkgdown/R/build-articles.R

Lines 157 to 180 in fc092a8

	#' By default, YAML option `as_is: true` sets `theme` to `NULL` internally.
	#' The default value of the argument `theme` is hardcoded in
	#' [rmarkdown::html_vignette()] too. So in order to enable the ability to use
	#' either:
	#'
	#' ```
	#' output:
	#' rmarkdown::html_vignette
	#' ```
	#' or
	#' ```
	#' output:
	#' bookdown::html_document2:
	#' base_format: rmarkdown::html_vignette
	#' ```
	#'
	#' in pkgdown, option `set_null_theme` should be set to `false` in YAML as
	#' follows:
	#' ```
	#' pkgdown:
	#' as_is: true
	#' set_null_theme: false
	#' ```
	#'

Or (2) any section/topic from this book?

(1) Is already implemented and (2), in my opinion, is out of the scope for pkgdown package.

[jayhesselberth]

Could you please add a minimal vignettes/test/bookdown.Rmd that tests for function of some bookdown features (e.g., cross-referencing)?

Edit: You'd have to name this _bookdown.Rmd so it doesn't build; pkgdown does not import bookdown.

[Robinlovelace]

@GegznaV yes that is what I had in mind. I've made the point that you can cross reference tables and figures with a link the the bookdown package here - seem reasonable?: https://github.com/forked-packages/pkgdown/pull/1

[GegznaV]

@jayhesselberth,

1. Could you please add a minimal...

   do you mean this: 6fe74d9?

2. Should my PR include documentation updates by @Robinlovelace's (in https://github.com/forked-packages/pkgdown/pull/1)
   UPDATED: I accepted these changes as mainty they mainly included a link to cross-references section in bookdown book.

[jayhesselberth]

@hadley this looks to be a useful addition that addresses #853 with minimal changes.

[Robinlovelace]

I fully support this PR. Here's a great example of a html_vignette2 that can be built thanks to this work: https://saferactive.github.io/trafficalmr/articles/report1.html

[hadley]

Why do we need to make this a parameter? Could we simply not reset the theme to true? (It's likely I'm missing something in the discussion; it would be very useful if someone could summarise (a) what precisely the problem is and (b) why this is the correct fix).

[Robinlovelace]

I think that if html_vignette2 could 'just work' that would be ideal. I have the impression that before this PR it didn't work so this is greatly appreciated but it still seems to require a few other things that were not totally intuitive to me, documented here: saferactive/trafficalmr@ee61673

[GegznaV]

@hadley

(a) what precisely the problem is

By default, pkgdown builds all articles with rmarkdown::html_document() and ignores some information in YAML header in vignettes (e.g., other formats). To enable other formats, pkgdown: as_is: true should be used (see https://pkgdown.r-lib.org/reference/build_articles.html#yaml-header for details).

If a vignette uses cross-references, autonumbering of figures and tables as well as other features from bookdown, rmarkdown::html_document() is not sufficient. And, e.g., bookdown::html_vignette2() supports these features. But the option pkgdown: as_is: true hardcodes the value of theme to NULL.
rmarkdown::html_vignette (the base format of bookdown::html_vignette2()) hardcodes this value to NULL too (see this documentaion for more details):

Please note that theme, fig_retina and highlight are hard coded.
Setting any of those will yield an error.

Thus rmarkdown::html_vignette + pkgdown: as_is: true results in error.

Error message

From this example #1352 (comment) if the current version of pkgdown is used:

> pkgdown::build_article("test-pkgdown-ok") 
Reading 'vignettes/test-pkgdown-ok.Rmd'
Error: Failed to render RMarkdown
* Error in html_document(fig_width = fig_width, fig_height = fig_height,  : 
*   formal argument "theme" matched by multiple actual arguments
Run `rlang::last_error()` to see where the error occurred.

> rlang::last_error()
<error/rlang_error>
Failed to render RMarkdown
* Error in html_document(fig_width = fig_width, fig_height = fig_height,  : 
*   formal argument "theme" matched by multiple actual arguments
Backtrace:
 1. pkgdown::build_article("test-pkgdown-ok")
 2. pkgdown:::render_rmarkdown(...)
Run `rlang::last_trace()` to see the full context.

> rlang::last_trace()
<error/rlang_error>
Failed to render RMarkdown
* Error in html_document(fig_width = fig_width, fig_height = fig_height,  : 
*   formal argument "theme" matched by multiple actual arguments
Backtrace:
    x
 1. \-pkgdown::build_article("test-pkgdown-ok")
 2.   \-pkgdown:::render_rmarkdown(...)
<error/callr_status_error>
callr subprocess failed: formal argument "theme" matched by multiple actual arguments
<error/callr_remote_error>
formal argument "theme" matched by multiple actual arguments
Backtrace:
x

---

So it is at least strange when one can have some features in a vignette that is distributed with regular R documentation but cannot have the same features reproduced in the pkgdown version of the same documentation.
I.e., I think it is a bug, that pkgdown does not support the default format of HTLM vignettes.

---

(b) why this is the correct fix

In this PR I do not question why certain pkgdown settings are used by default. I just provided a solution to a non-addressed problem and the solution:

• just minimally alters the code in pkgdown,
• does not alter any default settings (in order not to break something unintentionally).

Of course, It would be preferred if pkgdown supported rmarkdown::html_vignette by default with no additional parameters (e.g., as_is, etc.) needed to be set.

[hadley]

Thanks for the explanation. Why can't we just not hardcode theme when as_is: true?

(Due to the way that pkgdown has to integrate the website template with the RMarkdown template, I don't think that's it's possible to support general RMarkdown documents, but I can make carefully considered changes for v. popular formats.)

[GegznaV]

This as_is option was introduced in 094d9bf. Maybe the hardcoding has something to do with the option extension: pdf. Unfortunately, I do not have experience with PDF vignettes.

But for HTML, I set up an experiment.

1. I installed pkgdown from this pull request.
2. I git clone'd this repo and created a pkgdown website for it with the following options in the vignettes:
   a. I changed nothing in the vignettes;
   b. I used as_is: true (the current behavior of pkgdown when this option is used):

   pkgdown:
     as_is: true

   c. I used set_null_theme: no to remove the hardcoded NULL theme:

   pkgdown:
     as_is: true
     set_null_theme: no

The result of a is the current version of pkgdown's website, e.g., https://pkgdown.r-lib.org/dev/articles/linking.html
E.g.:

For b, the process resulted in the error due to hardcoded theme:

The result of c could be found in this zip: as_is-true set_null_theme--no.zip. The main difference between a and c is that the table of contents is missing in c:

To sum up, for the main format of HTML vignettes, I see no advantage of having a hardcoded NULL theme.

[GegznaV]

I created an alternative PR #1371 to see if it passes the tests on CI. In the PR, the hardcoding theme to NULL is removed.

[hadley]

Superseded by #1371