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

file_scope is now correctly merged when creating output_format #2488

Merged
merged 8 commits into from
Jul 11, 2023

Conversation

atusy
Copy link
Collaborator

@atusy atusy commented May 26, 2023

Currently, new output format ignores the file scope of the base format.
It is totally lost.

In this PR I fix the above behavior and add merge_file_scope().

To achieve merging, file_scope function needs two arguments, and the latter is new

  • input_files which already exists
  • current_file_scope which is determined by the base file_scope function

In this way, overlay can take into account of file scope determined by the base.

This is an API change, so I added some warnings and fallback mechanisms.

@atusy
Copy link
Collaborator Author

atusy commented May 26, 2023

Note that I noticed this when adding tests on #2462, which should be able to extend file_scope from within chunks.

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@yihui yihui requested a review from cderv May 26, 2023 01:39
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.

Sorry for the delay on this @atusy.

it looks good to me. I think it won't break current behavior. Only place that we use it in in bookdown. It is even deactivated by default, and could be completely broken in fact ... :(

I am just curious: do we have a use case where we think merging file_scope requires applying overlay on base results ?

To me this would mean that we want to define a format that would for example extend gitbook() but would split again each of the splitted file, instead of providing an overwrite of the split function. Is that right ?

Just trying to understand correctly. I guess someone can do a function that ignores current_scope to completely ignore the base splitted content.

I was thinking maybe the latter would be the desired behavior when extending the base format. So I was curious if this change was driven by a use case. Thanks.

Other than this question I have, the implementation looks very good ! thanks a lot !

@CLAassistant
Copy link

CLAassistant commented Jul 11, 2023

CLA assistant check
All committers have signed the CLA.

[skip ci]
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.

In fact it is all good. Both usage can be done. Let's go with this default. Thanks a lot !

@cderv cderv changed the title add missing functionality to merge file_scope on creating output_format file_scope is now correctly merged when creating output_format Jul 11, 2023
@cderv cderv merged commit 4b8e5c1 into rstudio:main Jul 11, 2023
@atusy
Copy link
Collaborator Author

atusy commented Jul 11, 2023

Actually I'm not sure if there would be any practical usecases...

One exception is test.
We can easily wrap testing output format and test if file_scope is working as expected.

test_that("file_scope replaces base format", {
  out_fmt <- output_format(
    knitr = NULL, pandoc = NULL, keep_md = NULL, clean_supporting = NULL,
    file_scope = function(input_file, file_scope, ...) {
      # do some tests here!
      testthat::expect_snapshot(cat(file_scope))
      return(file_scope)
    },
    base_format = bookdown::git_book
  )
  rmarkdown::render("example.Rmd", output_format = out_fmt)
})

@cderv
Copy link
Collaborator

cderv commented Jul 11, 2023

Good use case ! I had not thought of that 😆

jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Aug 16, 2023
* rstudio/main:
  start the next version
  CRAN release v2.24
  shinyrmd: Safer dependency extraction from pre-rendered HTML (rstudio#2500)
  quote the version number per CRAN's request
  Add output_format_dependency() (rstudio#2462)
  file_scope is now correctly merged when creating output_format (rstudio#2488)
  Correctly run some tests only on CI
  start the next version
  CRAN release v2.23
  remove broken links
  suggest cleanrmd for e499bf7
  add news
  comparing version numbers with numbers is no longer allowed: https://bugs.r-project.org/show_bug.cgi?id=18548
  `find_external_resources` works with custom format using `theme` (rstudio#2494)
  start the next version
  CRAN release v2.22
  S3 generic/method consistency
  Change the code-folding button text from "Code" to "Show" (rstudio#2489)
  fix: bump jquery-ui to v1.13.2 to fix multiple CVEs (rstudio#2477)
  detecting external resources needs to consider css argument (rstudio#2486)
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Aug 16, 2023
Merge remote-tracking branch 'rstudio/main' into jg-devel

# By Yihui Xie (13) and others
# Via Yihui Xie
* rstudio/main:
  start the next version
  CRAN release v2.24
  shinyrmd: Safer dependency extraction from pre-rendered HTML (rstudio#2500)
  quote the version number per CRAN's request
  Add output_format_dependency() (rstudio#2462)
  file_scope is now correctly merged when creating output_format (rstudio#2488)
  Correctly run some tests only on CI
  start the next version
  CRAN release v2.23
  remove broken links
  suggest cleanrmd for e499bf7
  add news
  comparing version numbers with numbers is no longer allowed: https://bugs.r-project.org/show_bug.cgi?id=18548
  `find_external_resources` works with custom format using `theme` (rstudio#2494)
  start the next version
  CRAN release v2.22
  S3 generic/method consistency
  Change the code-folding button text from "Code" to "Show" (rstudio#2489)
  fix: bump jquery-ui to v1.13.2 to fix multiple CVEs (rstudio#2477)
  detecting external resources needs to consider css argument (rstudio#2486)

# Conflicts:
#	DESCRIPTION
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Aug 16, 2023
* jg-devel: (21 commits)
  Updated NEWS. Patched `merge_output_format_dependency` to ensure that named elements remain in the correct order.
  start the next version
  CRAN release v2.24
  shinyrmd: Safer dependency extraction from pre-rendered HTML (rstudio#2500)
  quote the version number per CRAN's request
  Add output_format_dependency() (rstudio#2462)
  file_scope is now correctly merged when creating output_format (rstudio#2488)
  Correctly run some tests only on CI
  start the next version
  CRAN release v2.23
  remove broken links
  suggest cleanrmd for e499bf7
  add news
  comparing version numbers with numbers is no longer allowed: https://bugs.r-project.org/show_bug.cgi?id=18548
  `find_external_resources` works with custom format using `theme` (rstudio#2494)
  start the next version
  CRAN release v2.22
  S3 generic/method consistency
  Change the code-folding button text from "Code" to "Show" (rstudio#2489)
  fix: bump jquery-ui to v1.13.2 to fix multiple CVEs (rstudio#2477)
  ...
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants