rstudio / blogdown

Create Blogs and Websites with R Markdown
https://pkgs.rstudio.com/blogdown/
1.73k stars 334 forks source link

check_config() formatted for config.toml but not yaml #554

Closed apreshill closed 3 years ago

apreshill commented 3 years ago

Hi @yihui -

Comments on my blog post revealed an issue with check_config(), specifically the ignore files, filing here!

https://github.com/rbind/apreshill/issues/44#issuecomment-753672364

https://github.com/rbind/apreshill/issues/44#issuecomment-753689876

yihui commented 3 years ago

The character array syntax ["a", "b"] works for TOML, YAML, and JSON. https://github.com/rbind/apreshill/issues/44#issuecomment-753672364 was bitten by the quoting rules of YAML. Without double quotes, \\ means literally two backslashes.

> yaml::yaml.load('- \\\\.')
[1] "\\\\."
> yaml::yaml.load('- "\\\\."')
[1] "\\."
> yaml::yaml.load("- '\\\\.'")
[1] "\\\\."
akseong commented 3 years ago

That second comment is me. I'm terrible with regex, and it's messy, but I think changing:

https://github.com/rstudio/blogdown/blob/d04fd17ce152e607ecf4f63e7e139395bc027316/R/check.R#L49

to

check_todo('Set "ignoreFiles" to ', 
           ifelse(grepl(".yaml", f), 
                  gsub('\\\\\\\\', "\\\\",  xfun::tojson(ignore)), 
                  xfun::tojson(ignore)))

and also

https://github.com/rstudio/blogdown/blob/d04fd17ce152e607ecf4f63e7e139395bc027316/R/check.R#L53

to

ifelse(grepl(".yaml", f), 
       gsub('\\\\\\\\', "\\\\",  gsub('^\\[|\\]$', '', xfun::tojson(I(setdiff(ignore, s))))), 
       gsub('^\\[|\\]$', '', xfun::tojson(I(setdiff(ignore, s)))))

would do it?

edit: didn't see Yihui's comment. ok, nevermind!

yihui commented 3 years ago

I'm terrible with regex

@akseong Obviously not at all---you are able to understand the meaning of eight(!) backslashes in regex, and that alone indicates that you are already a reg(ex){2}pert!

apreshill commented 3 years ago

So the solution seems to indicate that the default config.yaml that is produced when creating a new Academic starter site with blogdown is not correct?

Here is what was created for me "out of the box":

https://github.com/apreshill/apreshill-phonic/blob/429e65b6dea59416882060d7d522a6dd6c671b7a/config.yaml#L15-L20

But the solution seems to be either:

ignoreFiles:
  - \.ipynb$
  - .ipynb_checkpoints$
  - \.Rmd$
  - \.Rmarkdown$
  - _cache$

Or:

ignoreFiles:
  - "\\.ipynb$"
  - ".ipynb_checkpoints$"
  - "\\.Rmd$"
  - "\\.Rmarkdown$"
  - "_cache$"

Is it possible that when blogdown converts the config.toml to config.yaml, it is not properly formatted?

yihui commented 3 years ago

@apreshill It seems that you have the RcppTOML package installed, and it turns out that it has a bug (two backslashes inside double quotes should mean a single literal backslash):

> # the result is wrong
> RcppTOML::parseTOML('a = ["\\\\foo"]', fromFile = F)
List of 1
 $ a: chr "\\\\foo"

> # the yaml package is correct
> yaml::yaml.load('a: ["\\\\foo"]')
$a
[1] "\\foo"

I just pushed a fix to make sure the conversion from TOML to YAML is correct. Thanks!

cderv commented 3 years ago

@yihui I don't think this is a bug. From ?RcppTOML::parseTOML, there is an escape argument that default to TRUE for backward compatibility. It seems it was not handling correctly at first, and you need to use escape = FALSE now

RcppTOML::parseTOML('a = ["\\\\foo"]', fromFile = F, escape = FALSE)
#> List of 1
#>  $ a: chr "\\foo"

Created on 2021-01-05 by the reprex package (v0.3.0.9001)

yihui commented 3 years ago

@cderv Oh I didn't know that. Just added escape = FALSE. Thanks for the tip!