-
-
Notifications
You must be signed in to change notification settings - Fork 982
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
Pass parsed metadata into pre_knit #2485
Conversation
They will be read before pre_knit and access can be useful to do some side effect based on metadata in file. (use case in rticles) Current workaround is to re-parse the YAML from the input file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! My only concern is whether all people have really followed the advice in the documentation to add the ...
argument to their pre_knit
function. We'll know when we do the revdeps check.
yes I have the same concern. We'll see. |
Maybe we can test if If not, we can fallback to the previous behavior with warning. names(formals(function(x, ...) x))
#> [1] "x" "..." |
This warning will also be helpful when we add more arguments in the future. |
@yihui did you ran a reverse dependency check on that yet ? I thought we would try merge it before next release and see if all passes but I think you have done the release already. I'll run one on this for next release if you haven't done it. |
Let's merge this after the next release (which is on its way). Thanks! |
Sorry I forgot this PR. I just started the revdeps check: https://github.com/yihui/crandalf/actions/runs/6198690012/job/16829628626 I'm hoping to make a new release in the next few days if this PR works for revdeps. |
Revdep checks are finished. Only one package would be broken: https://github.com/line/conflr/blob/74c3c36f0a786138d4d90f49df45ab165901761c/R/document.R#L133 Good news is that this package has been retired. Anyway, I'll use @atusy's idea above to test for the existence of |
…one or two arguments to it
They will be read before pre_knit and access can be useful to do some side effect based on metadata in file. (use case in rticles)
Current workaround is to re-parse the YAML from the input file using
rmarkdown::yaml_front_matter()
This is easy enough though, so maybe it is ok to keep it that way.
I opened the PR to share the idea as we always told in our doc that
pre_knit
should support...
for future expansion, and it seems ok to addWhat do you think ? Did I miss something that would prevent us doing this ?