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

Use new version of has_crop_tools() from knitr #2532

Merged
merged 7 commits into from
Dec 11, 2023
Merged

Use new version of has_crop_tools() from knitr #2532

merged 7 commits into from
Dec 11, 2023

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Dec 11, 2023

Context:

We improved the logic in knitr but never ported to rmarkdown.

I use conditional to use internal knitr function.

  • Should it be exported from knitr instead ?
  • Should we inline instead to replace the one in rmarkdown completely ?
  • Or is it old enough so that we bump knitr min requirement ?

It handles new windows specificity regarding ghostscript

Context at yihui/knitr#2246
@cderv cderv requested a review from yihui December 11, 2023 15:37
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.

  • Should it be exported from knitr instead ?
  • Should we inline instead to replace the one in rmarkdown completely ?
  • Or is it old enough so that we bump knitr min requirement ?

I think it's fine to use ::: (CRAN allows for it when the maintainer is the same for the two packages) and not export it from knitr.

I'm also okay with requiring knitr >= 1.43. In theory, when users install the new version of rmarkdown, knitr should be updated automatically if we specify this version requirement. In practice, I did see (rare) cases in which the version requirement was bypassed, but I have never understood how it was possible.

@cderv
Copy link
Collaborator Author

cderv commented Dec 11, 2023

Ok I went with the dependency upgrade.

I did not use the function directly. Not sure if any of our others tools is using rmarkdown:::has_crop_tools() so I went with just using knitr internal in it.

We could have re-export but don't think that works with internal function as I would have done

#' @importFrom knitr has_crop_tools
#' @export
knitr::has_crop_tools

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. Thanks!

@yihui yihui merged commit 7187bd8 into main Dec 11, 2023
@yihui yihui deleted the knitr-pdf-crop branch December 11, 2023 17:23
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Dec 19, 2023
Merge remote-tracking branch 'rstudio/main' into jg-tree-fix

* rstudio/main:
  Use new version of `has_crop_tools()` from knitr (rstudio#2532)
  some cosmetic changes
  use ignore.case = TRUE for regex functions instead of enumerating upper/lowercase letters
  Remove `stringr` dependency (rstudio#2530)
  Add a mention about required configuration when erroring about webshot and webshot2

# Conflicts:
#	NEWS.md
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Dec 19, 2023
Merge remote-tracking branch 'rstudio/main' into jg-devel

* rstudio/main:
  Use new version of `has_crop_tools()` from knitr (rstudio#2532)
  some cosmetic changes
  use ignore.case = TRUE for regex functions instead of enumerating upper/lowercase letters
  Remove `stringr` dependency (rstudio#2530)
  Add a mention about required configuration when erroring about webshot and webshot2

# Conflicts:
#	NEWS.md
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Dec 19, 2023
Merge branch 'jg-devel'

* jg-devel:
  Use new version of `has_crop_tools()` from knitr (rstudio#2532)
  some cosmetic changes
  use ignore.case = TRUE for regex functions instead of enumerating upper/lowercase letters
  Remove `stringr` dependency (rstudio#2530)
  Add a mention about required configuration when erroring about webshot and webshot2
  Fixed documentation problems with `publish_site()`

# Conflicts:
#	R/publish_site.R
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 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.

2 participants