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

Export parse_params() for developers of functionality in third-party packages adjacient to {knitr} #2112

Closed
3 tasks done
lorenzwalthert opened this issue Mar 11, 2022 · 8 comments
Labels
next Issues/PRs considered for the next release

Comments

@lorenzwalthert
Copy link
Contributor

lorenzwalthert commented Mar 11, 2022

By filing an issue to this repo, I promise that

  • I have fully read the issue guide at https://yihui.org/issue/.
  • 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.
  • 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.


I'd ask you to export parse_params() so other projects that are adjacient to {knitr} can leverage that functionality in their developing efforts. This would most likely make the end user experience more consistent and reduce code duplication. In particular, in {styler}, we currently parse the parameters ourself to:

{knitr} already expors some functionality that most end users don't need, like sew(), all_patterns and more, so exporting parse_params() be in line with current API policies. There are most likely other packages that would benefit in the long run, plus the API of that function is probably quite established and stable.

This came up in r-lib/styler#928.

@yihui
Copy link
Owner

yihui commented Mar 24, 2022

Exporting the current parse_params() is not enough. It could help you if users specify the engine option, but for R Markdown users, the engine is usually specified directly after the three backticks, i.e., ```{Rcpp} instead of ```{r, engine="Rcpp"}.

I'll need to move some code from parse_block().

Anyway, I'm okay with exporting these low-level functions, but probably not very soon.

@yihui yihui added the next Issues/PRs considered for the next release label Mar 24, 2022
@lorenzwalthert
Copy link
Contributor Author

Ok thanks, sounds good 👍

@yihui
Copy link
Owner

yihui commented Mar 24, 2022

Actually, I feel that exporting parse_params() won't really help you with the two problems. I don't know much about how styler parses knitr documents, but I feel parse_params() is not what you want to use. To make sure, you can start with knitr:::parse_params (triple-colon). If it can solve your problem, I can certainly export it, but I doubt so.

@lorenzwalthert
Copy link
Contributor Author

lorenzwalthert commented Jul 26, 2022

Ok, I think I get what you mean now, i.e. if engine is not set it is parsed as a label in my understanding:

knitr:::parse_params("a=1, engine='Rcpp'")
#> $a
#> [1] 1
#> 
#> $engine
#> [1] "Rcpp"
#> 
#> $label
#> [1] "unnamed-chunk-3"

knitr:::parse_params("Rcpp, foo, a=1,")
#> $label
#> [1] "Rcpp, foo"
#> 
#> $a
#> [1] 1

Created on 2022-03-10 by the reprex package (v2.0.1.9000)

Is that the problem you said we'll encounter? Of course, if we can get

knitr:::parse_params("Rcpp, foo, a=1,")
#> $engine
#> [1] "Rcpp"
#> $label
#> [1] "foo"
#> 
#> $a
#> [1] 1

from parse_params(), that would make our life easier. Can we safely assume that if engine is not returned, everything in label up to the first comma is the engine?

@yihui
Copy link
Owner

yihui commented Jul 28, 2022

If you only work with R Markdown documents, you can preprocess the chunk header before passing it to parse_params():

params = sub('^([a-zA-Z0-9_]+)(.*)$', 'engine="\\1",\\2', params)

Yes, for R Markdown, you can safely assume the first string is always the engine.

@lorenzwalthert
Copy link
Contributor Author

ok, great. Thanks for the regex. {styler} styles .R, .Rprofile, .Rmd, .Rmarkdown, .qmd and .Rnw and IIRC .Rnw can only contain R code so I think we've got everything covered.

@yihui
Copy link
Owner

yihui commented Aug 11, 2022

Great! Just a small correction: .Rnw can contain other languages, too (use the chunk option engine).

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
next Issues/PRs considered for the next release
Projects
None yet
Development

No branches or pull requests

2 participants