Update to knitr breaks quarto?

giabaio commented 1 year ago

I believe that one of the latest changes in knitr breaks quarto (see this). I think the problem is that commit #2443 has reverted the code to a small inconsistency (as discussed in #2248).

Downgrading knitr to a prior version sorts the issue, though...

I hope this is helpful?

By filing an issue to this repo, I promise that

[X] I have fully read the issue guide at https://yihui.org/issue/.
[X] I have provided the necessary information about my issue.
- If I'm asking a question, I have already asked it on Stack Overflow or RStudio Community, waited for at least 24 hours, and included a link to my question there.
- If I'm filing a bug report, I have included a minimal, self-contained, and reproducible example, and have also included xfun::session_info('knitr'). I have upgraded all my packages to their latest versions (e.g., R, RStudio, and R packages), and also tried the development version: remotes::install_github('yihui/knitr').
- If I have posted the same issue elsewhere, I have also mentioned it in this issue.
[X] I have learned the Github Markdown syntax, and formatted my issue correctly.

I understand that my issue may be closed if I don't fulfill my promises.

cderv commented 1 year ago

Indeed #2243 has broken quarto, but not related to #2248 directly.

@yihui I am available to discuss this. This is the addition of a new argument in add_html_caption() that breaks quarto, because of its usage in Quarto code base too.

Either something to fix in Quarto, or we should take more into knitr how Quarto is using our internals functions in a specific way.

yihui commented 1 year ago

Like I said last time, we should make as less use of assignInNamespace() as possible in Quarto. Modifying a package's internal functions is bad practice in general. If that's really unavoidable, at least Quarto needs to test against the dev version of knitr so that we can discover such problems early.

We could move some R code from Quarto to knitr conditioned on is_quarto().

yihui commented 10 months ago

It's great that Quarto is being tested against the dev version of knitr now. I wonder if there's anything else we could do for this issue.

cderv commented 10 months ago

I think initially I left that open regarding https://github.com/quarto-dev/quarto-cli/pull/5704#issuecomment-1564449839

We fixed in quarto for 1.3 (patch) and next 1.4 but their could have been issue with user having oldest Quarto (bundled in RStudio for example) and updating knitr.

This issue here so that we could see if something (workaround conditional to quarto version maybe ?) was needed. It seems there was no more report of this so maybe we're good.

github-actions[bot] commented 4 months ago

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.

yihui / knitr

Update to knitr breaks quarto? #2261