-
Notifications
You must be signed in to change notification settings - Fork 11
Description
fig.path
has quite specific undocumented requirements and can fail in corner cases.
MRE:
litedown::reactor(fig.path = "foo")
litedown::fuse(text = "```{r} \n plot(1) \n```")
File '.foochunk-1-1.png' not found (hence cannot be embedded).
foochunk-1-1.png
is created- the path correction in
fuse
fails and appends a dot for a relative-path (instead of e.g.,./
) fig.path
is not actually a path, but a path with prefix, e.g.,- the requirement of "/" is not documented
When fig.path
contains /
, it works.
litedown::reactor(fig.path = "foo/bar")
litedown::fuse(text = "```{r} \n plot(1) \n```", output = "markdown")
produces 
The problem likely lies in:
Lines 480 to 493 in 7846749
set_path = function(name) { | |
# fig.path = output__files/ if `output` is a path, otherwise use | |
# litedown__files/ (we don't use _files because of rstudio/rmarkdown#2550) | |
if (is.null(p <- opts[[name]])) p = aux_path(output_base %||% 'litedown', name) | |
slash = endsWith(p, '/') | |
# make sure path is absolute so it will be immune to setwd() (in code chunks) | |
if (is_rel_path(p)) { | |
p = file.path(getwd(), p) | |
# preserve trailing slash because file.path() removes it on Windows | |
if (slash) p = sub('/*$', '/', p) | |
} | |
opts[[name]] = p | |
} | |
set_path('fig.path'); set_path('cache.path') |
IMHO, it feels like uneccesary complex code for just setting path, which is further complicated by it being an inner function.
is_rel_path
itself has several layers of indirection.
Suggestion:
- Make
set_path
a global function instead of an inner function, and make it a true function (if possible), not depending on local objects - make test for desired behaviour (there seems to be some history that is not documented with tests)
Discussion:
Is fig.path
serving as a path prefix (instead of directory itself) a desired behaviour? It allows some nice things, but it making the behaviour quite a bit complicated from programming and user perspective (such as, the undocumented requirement for "/" if fig.path
is really a path.
If you are happy about this suggestion, I will implement it.