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

HTML5 fixes #1290

Closed
hadley opened this issue Feb 18, 2022 · 6 comments
Closed

HTML5 fixes #1290

hadley opened this issue Feb 18, 2022 · 6 comments
Milestone

Comments

@hadley
Copy link
Member

hadley commented Feb 18, 2022

From Kurt Hornik

  • the (new) warnings about " attribute "align" not allowed for
    HTML5" are typically from roxygen2 doing

    ./R/object-defaults.R:    fig <- "\\if{html}{\\figure{logo.png}{options: align='right' alt='logo' width='120'}}"
    

    This can easily be fixed by changing align='right' to

    style='text-align: right;'
    
  • another group of problems is from putting R code in examples into a
    div. One test case is keras/man/timeseries_dataset_from_array.Rd
    which has

    Consider an array \code{data} of scalar values, of shape \code{(steps)}.
    To generate a dataset that uses the past 10
    timesteps to predict the next timestep, you would use:\if{html}{\out{<div class="sourceCode R">}}\preformatted{steps <- 100
    

    which does not work "as intended", as \preformatted will add a </p> to
    close the preceding paragraph but after the inserted <div>.

    Afaict, we don't have a way to force closing a paragraph other than
    adding an empty line, so changing the generated Rd to

    Consider an array \code{data} of scalar values, of shape \code{(steps)}.
    To generate a dataset that uses the past 10
    timesteps to predict the next timestep, you would use:\if{html}{
    
    \out{<div class="sourceCode R">}}\preformatted{steps <- 100
    

    will "work" (and also not add an extra line in the LaTeX version):
    could you please change roxygen2 accordingly?

  • Finally, most warnings are from generating methods lists for R6
    classes. E.g., for webmockr/man/Adapter.Rd we get inherited methods
    entries like

     <li> <span class="pkg-link" data-pkg="webmockr" data-topic="Adapter" data-id="disable"><p><a href="../../webmockr/html/Adapter.html%23method-disable"><code>webmockr::Adapter$disable()</code></a></span>
     </p>
     </li>

    where the span starts outside the p but is closed inside the p.

    I don't see a "simple" fix for this, but the code in rd-r6.R is

    c("\\if{html}{",
      paste0("\\out{", details, "}"),
      "\\itemize{",
      sprintf(
        paste0(
          "\\item \\out{<span class=\"pkg-link\" data-pkg=\"%s\" ",
          "data-topic=\"%s\" data-id=\"%s\">}",
          "\\href{../../%s/html/%s.html#method-%s}{\\code{%s::%s$%s()}}",
          "\\out{</span>}"
        ),
        super_meth$package,
        super_meth$classname,
        super_meth$name,
        super_meth$package,
        super_meth$classname,
        super_meth$name,
        super_meth$package,
        super_meth$classname,
        super_meth$name
      ),
      "}",
      "\\out{</details>}",
      "}"
      )
    }
    

    so perhaps instead of calling \href (which will start the p inside the
    span) you could simply \out the a hyperlink directly?

@hadley
Copy link
Member Author

hadley commented Feb 21, 2022

  • There are a few instances of multiply defined anchors. E.g., for
    readr/man/callback.Rd, one get

    callback.Rd:\item \href{#method-new}{\code{ChunkCallback$new()}}
    callback.Rd:\if{html}{\out{<a id="method-new"></a>}}
    callback.Rd:\if{latex}{\out{\hypertarget{method-new}{}}}
    callback.Rd:\item \href{#method-new}{\code{SideEffectChunkCallback$new()}}
    callback.Rd:\if{html}{\out{<a id="method-new"></a>}}
    callback.Rd:\if{latex}{\out{\hypertarget{method-new}{}}}
    callback.Rd:\item \href{#method-new}{\code{DataFrameCallback$new()}}
    callback.Rd:\if{html}{\out{<a id="method-new"></a>}}
    callback.Rd:\if{latex}{\out{\hypertarget{method-new}{}}}
    callback.Rd:\item \href{#method-new}{\code{ListCallback$new()}}
    callback.Rd:\if{html}{\out{<a id="method-new"></a>}}
    callback.Rd:\if{latex}{\out{\hypertarget{method-new}{}}}
    callback.Rd:\item \href{#method-new}{\code{AccumulateCallback$new()}}
    callback.Rd:\if{html}{\out{<a id="method-new"></a>}}
    callback.Rd:\if{latex}{\out{\hypertarget{method-new}{}}}
    

    with several 'method-new' anchors. Can you pls take a look?

  • For e.g. tidyquant/man/excel_if_functions.Rd one gets

    \emph{Create your own summary "\emph{IF" function}}
    

    with a nested \emph, but I guess the roxygen markup is not as
    intended?

  • mctq/R/sdu.R has

    #' ## For standard and micro versions of the MCTQ
    #'
    #' __\deqn{SE_{W/F} - SO_{W/F}}{SE_W/F - SO_W/F}__
    

    given a \deqn inside an \emph: could this be avoided?

  • hadron/R/cvc_readutils.R has

    #' @title Generate HDF5 key for CVC 'correlators' meson 3pt function with a local or derivative insertion
    #' 
    #' The key for a meson three-point function has the form:
    

    so the generated \title contains the whole Rd file. Could this be
    avoided?

    (Alternatively, I could look into complaining about multi-paragraph
    titles.)

@hadley hadley added this to the v7.1.3 milestone Mar 29, 2022
@hadley
Copy link
Member Author

hadley commented Apr 6, 2022

To do:

@hadley
Copy link
Member Author

hadley commented Apr 19, 2022

check_html5 <- function(path) {
  html <- withr::local_tempfile()
  tools::Rd2HTML(path, html)
  
  processx::run(
    "/opt/homebrew/Cellar/tidy-html5/5.8.0/bin/tidy",
    c("-eq", html),
    echo = TRUE
  )
}
purrr::walk(dir("man", full.names = TRUE, pattern = "\\.Rd$"), check_html5)

@hadley
Copy link
Member Author

hadley commented Apr 19, 2022

Or using htmltidy package:

check_html5 <- function(path) {

  html <- withr::local_tempfile()
  tools::Rd2HTML(path, html)

  opts <- list(
    TidyDoctype = "html5",
    TidyHtmlOut = TRUE
  )
  out <- capture.output(
    htmltidy::tidy_html(file(html), verbose = TRUE, options = opts)
  )

  out <- out[grepl("^line ", out)]
  if (length(out)) {
    cli::cli_abort(c("{.path {path}} is not tidy", out))
  }
  invisible()
}

@hadley hadley closed this as completed Apr 24, 2022
@hadley
Copy link
Member Author

hadley commented Apr 24, 2022

I'm pretty confident that this is now correct. I don't particularly want to add tests using either the command line app (because of the challenges installing it on different platforms) or the package (because of the high dependencies, and ugliness of my wrapper).

@gaborcsardi
Copy link
Member

We could also have an "extra" test, that we could run manually, and then that could use a command line tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants