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

Restore opts_current after each code chunk, and also after knit() exits #2292

Merged
merged 5 commits into from
Oct 25, 2023

Conversation

yihui
Copy link
Owner

@yihui yihui commented Sep 21, 2023

Will this fix #1988? (except for the support of inline labels)

@yihui yihui requested a review from cderv September 21, 2023 21:04
@yihui yihui self-assigned this Sep 21, 2023
@yihui yihui mentioned this pull request Sep 21, 2023
3 tasks
Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I believe this is what is needed. This means that Inline code won't have any label.
We can think of something later; but this will probably make some existing code fails.

See related magick issue: ropensci/magick#310
It is using inline code to insert an image and rely on the fact that knitr will provide a name to a file when using plot_counter(). Previously it was using previous chunk name and overriding the previous image, but now it will have no name.

When running the example there, I see for the inline code

  • options$fig.path being figure/ instead of test_files/figure-gfm/. Not sure why this options is different.. 🤔
  • No label so valid_path(options$fig.path, options$label) is only figure/. Then suffix -1 will apply

It seems opts_chunk$get() and opts_current$get() are not the same.

Should opts_current in inline code be set to opts_chunk maybe ? Like in chunk ?
Inline have no params, but they should probably default to the global options that could have been modified ?

diff --git a/R/block.R b/R/block.R
index 35bbed87..12c486dc 100644
--- a/R/block.R
+++ b/R/block.R
@@ -547,6 +547,8 @@ merge_character = function(res) {
 }

 call_inline = function(block) {
+  optc = opts_current$get(); on.exit(opts_current$restore(optc), add = TRUE)
+  opts_current$restore(opts_chunk$get())
   if (opts_knit$get('progress')) print(block)
   in_input_dir(inline_exec(block))
 }

We could probably also deal with adding a label this way IMO

diff --git a/R/block.R b/R/block.R
index 35bbed87..4e1c5c80 100644
--- a/R/block.R
+++ b/R/block.R
@@ -547,6 +547,9 @@ merge_character = function(res) {
 }

 call_inline = function(block) {
+  optc = opts_current$get(); on.exit(opts_current$restore(optc), add = TRUE)
+  params = opts_chunk$merge(list(label = unnamed_chunk("inline", inline_counter())))
+  opts_current$restore(params)
   if (opts_knit$get('progress')) print(block)
   in_input_dir(inline_exec(block))
 }
diff --git a/R/utils.R b/R/utils.R
index 404dd768..271dae05 100644
--- a/R/utils.R
+++ b/R/utils.R
@@ -17,6 +17,7 @@ knit_counter = function(init = 0L) {
 plot_counter = knit_counter(1L)
 shot_counter = knit_counter(1L)
 chunk_counter = knit_counter(1L)
+inline_counter = knit_counter(1L)

 # a vectorized and better version than evaluate:::line_prompt
 line_prompt = function(x, prompt = getOption('prompt'), continue = getOption('continue')) {

I know using Images in inline code is not the best usage - problaby magick could be fixed to correctly handle inlines.

However, just ideas that you can tweak if you are interested,a s it seems setting label and global options as default for inline seems what should be to me.

@rundel this will partially solve #1988 as discussed.

@gadenbuie this should avoid the need to do this yourself (#1798 (comment))

yihui added a commit to yihui/knitr-examples that referenced this pull request Oct 25, 2023
yihui added a commit to yihui/knitr-examples that referenced this pull request Oct 25, 2023
@yihui
Copy link
Owner Author

yihui commented Oct 25, 2023

I've applied your patch with a small change. Thanks!

Now inline code will have access to a unique label in opts_current$get('label').

@yihui yihui merged commit a005847 into master Oct 25, 2023
9 checks passed
@yihui yihui deleted the restore-opts_current branch October 25, 2023 18:53
@cderv
Copy link
Collaborator

cderv commented Oct 25, 2023

Cool thanks !

I wonder how many side effect this unamed chunk scheme could cause 😅 it is possible that some user may rely on it too much. Would it invalidate existing cache too ?

Not saying it is bad, but we could probably communicate ahead of time if this could help

@yihui
Copy link
Owner Author

yihui commented Oct 25, 2023

Yes, this could be a breaking change, so I've started the revdep check: https://github.com/yihui/crandalf/actions/runs/6644880330/job/18054982641 I'll see how bad this change could be, and write a blog post accordingly.

yihui added a commit that referenced this pull request Oct 26, 2023
… exits (#2292)

also add labels to `opts_current` for inline code to fix #1988
@yihui
Copy link
Owner Author

yihui commented Oct 26, 2023

I've reverted the change in the naming scheme, since the revdep check showed that a few packages would be broken.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inline code chunks inheriting labels
2 participants