Skip to content

Incorrect parsing of fig.path #88

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

Open
J-Moravec opened this issue Apr 29, 2025 · 1 comment
Open

Incorrect parsing of fig.path #88

J-Moravec opened this issue Apr 29, 2025 · 1 comment

Comments

@J-Moravec
Copy link

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).

  1. foochunk-1-1.png is created
  2. the path correction in fuse fails and appends a dot for a relative-path (instead of e.g., ./)
  3. fig.path is not actually a path, but a path with prefix, e.g.,
  4. 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 ![](<foo/barchunk-1-1.png>)

The problem likely lies in:

litedown/R/fuse.R

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:

  1. Make set_path a global function instead of an inner function, and make it a true function (if possible), not depending on local objects
  2. 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.

J-Moravec added a commit to J-Moravec/litedown that referenced this issue May 14, 2025
@J-Moravec
Copy link
Author

Fixed by #94, the issue wasn't actually in the set_path, but later when the a path is derived from fig.path in fuse_code.

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

1 participant