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

Support including (R)md files into a manual page #902

Merged
merged 19 commits into from Sep 12, 2019
Merged

Conversation

@gaborcsardi
Copy link
Collaborator

@gaborcsardi gaborcsardi commented Sep 10, 2019

This facilitates sharing text between README/vignettes and manual pages.

TODO:

  • Handle sections/headings properly.
  • Add docs.
  • Add more tests.
  • Add to NEWS.
  • Allow easy/automatic caching.
Copy link
Member

@hadley hadley left a comment

This is a surprisingly small amount of code!

Can you please add a bullet to news, and some documentation to the rd vignette? I think the most important things to mention are what the path is relative to (the package root?), that the Rmd is copied to a temporary directory (so you can't rely on local files), and that it's not cached (so you'll need to be careful to make it fast).

I think it closes #669, #696, and #799.

.Rbuildignore Show resolved Hide resolved
R/rd-include-rmd.R Outdated Show resolved Hide resolved
@gaborcsardi
Copy link
Collaborator Author

@gaborcsardi gaborcsardi commented Sep 10, 2019

that the Rmd is copied to a temporary directory (so you can't rely on local files), and that it's not cached (so you'll need to be careful to make it fast).

Yeah, let me think about this, it would be great to have caching by default. Unfortunately we need to make a copy to make the links to package objects work.

@gaborcsardi
Copy link
Collaborator Author

@gaborcsardi gaborcsardi commented Sep 10, 2019

As for the sectioning, I think it would be reasonable to do this:

  • if there are no headings in the Rmd, then put the whole generated Rd into \details{}.
  • if there are subsections in the Rmd (i.e. ## and below), then create subsections inside \details{}.
  • level 1 headings, i.e. #, go in their own \section{}s.
  • if there is a level 1 heading, but not at the beginning of the Rmd file, then the beginning of the file goes to \details{}, up to the first #, and then we create new sections.

I think this is pretty intuitive, and most of often this is what you want.

@gaborcsardi gaborcsardi force-pushed the feature/rmd-import branch 2 times, most recently from 0ab3a00 to 3e0bcc2 Sep 11, 2019
@gaborcsardi
Copy link
Collaborator Author

@gaborcsardi gaborcsardi commented Sep 11, 2019

@hadley What should be the working directory? The package dir? The directory of the R file being processed?

Right now document() and roxygenize() do not change the working directory AFAICT. rmarkdown/knitr change it to the directory of the Rmd file.

We could change the working directory or try calculate the file name relative to the package directory. The former is simpler.

@hadley
Copy link
Member

@hadley hadley commented Sep 11, 2019

I think we should consistently use the package directory as the working directory

@gaborcsardi
Copy link
Collaborator Author

@gaborcsardi gaborcsardi commented Sep 11, 2019

I think we should consistently use the package directory as the working directory

OK, for now I'll switch for @includeRmd and then we can think about switching for document() and/or roxygenize().

@gaborcsardi
Copy link
Collaborator Author

@gaborcsardi gaborcsardi commented Sep 11, 2019

Btw. @includeRmd has proper support for sections now. We could have the same for "regular" roxygen2 markdown, we just need to turn it now, basically. Then we could use # etc. for (sub)sectioning.

Re #799, at this point it is super easy to add an @rmd tag as well, basically the same as @includeRmd() but takes the text from the block, instead of the external file. It would need some conventions where the actual Rmd files are created to support caching, whether they would share an environment, etc. So I will not do this now, but I think we could keep #799 open and have another PR for it later.

@gaborcsardi
Copy link
Collaborator Author

@gaborcsardi gaborcsardi commented Sep 11, 2019

Actually, re paths, since rmarkdown/knitr defaults to set the wd to the directory of the Rmd, I think we should do the same, for the chunks of the Rmd. We would still switch to the package directory for running rmarkdown::render(), so the paths you specify in @includeRmd would be relative to that. Is this OK?

Copy link
Member

@hadley hadley left a comment

I'm happy for this to close #799 — I'd like to live with this for a while before we consider more changes.

I was thinking mostly about the path to the .Rmd, not paths from within the .Rmd. I know there is a bunch of subtle behaviour in knitr when you change the working directory, so it might break with complex includes (or maybe outputs?), but I think that's fine for a first pass.

Should be documented with a new section in https://roxygen2.r-lib.org/articles/rd.html#do-repeat-yourself

R/markdown-link.R Outdated Show resolved Hide resolved
R/markdown.R Show resolved Hide resolved
R/rd-include-rmd.R Outdated Show resolved Hide resolved
R/rd-include-rmd.R Outdated Show resolved Hide resolved
R/rd-include-rmd.R Outdated Show resolved Hide resolved
R/rd.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
@gaborcsardi
Copy link
Collaborator Author

@gaborcsardi gaborcsardi commented Sep 11, 2019

I was thinking mostly about the path to the .Rmd

Yes, that's now relative to the package.

I'll update and add the missing stuff in a minute....

@gaborcsardi
Copy link
Collaborator Author

@gaborcsardi gaborcsardi commented Sep 11, 2019

@hadley Fixed all I think. Pls take a look at the text in the vignette, and whether it is at the best place.

@hadley
Copy link
Member

@hadley hadley commented Sep 11, 2019

Can you please merge/rebase fix the failure?

@gaborcsardi gaborcsardi force-pushed the feature/rmd-import branch from c3b55de to 0bdd879 Sep 11, 2019
@gaborcsardi
Copy link
Collaborator Author

@gaborcsardi gaborcsardi commented Sep 11, 2019

Rebased, but I think it'll still fail, because there is no pandoc.

@hadley
Copy link
Member

@hadley hadley commented Sep 11, 2019

Filed an issue: r-lib/r-azure-pipelines#5

@hadley
Copy link
Member

@hadley hadley commented Sep 12, 2019

Can you please merge/rebase once more and add skip_if_not(rmarkdown::pandoc_available()) so the azure builds pass?

@gaborcsardi
Copy link
Collaborator Author

@gaborcsardi gaborcsardi commented Sep 12, 2019

Yeah, a minute. knitr/pandoc also does escaping, so this PR needs an update or at least additional tests, now that the escaping in different in roxygen.

@gaborcsardi gaborcsardi force-pushed the feature/rmd-import branch from b36812b to f424b08 Sep 12, 2019
@gaborcsardi
Copy link
Collaborator Author

@gaborcsardi gaborcsardi commented Sep 12, 2019

Rebased, and tests are now skipped.

But wait, you added some commits again, let me rebase again....

gaborcsardi added 3 commits Sep 12, 2019
All Rmd is added to the \details{} section now.
Sections/headers are not (yet) handled.
So that the markdown parser will be able to create
sections for these.
@gaborcsardi gaborcsardi force-pushed the feature/rmd-import branch from 2a0f6eb to cef8d88 Sep 12, 2019
@gaborcsardi
Copy link
Collaborator Author

@gaborcsardi gaborcsardi commented Sep 12, 2019

OK, rebased again.

gaborcsardi added 4 commits Sep 12, 2019
Rules are:
- text before the first (level 1) heading goes into \details{}.
- level 1 headings get their own \section{}.
- other levels creates subsections, and go into \details{} or
  a \section{}, depending on whether they appear before the
  first level 1 heading or not.
- use a child document instead of a full copy
- set cache.path and fig.path to the default
  of the original Rmd, so caching works out of
  the box
- still need to turn caching on in the Rmd
- `@includeRmd` path is relative for the package root
- knitr's `root.dir` is set to the directory of the
  original Rmd. (I.e. _not_ the wrapper Rmd that we
  create.)
@gaborcsardi gaborcsardi force-pushed the feature/rmd-import branch from cef8d88 to 1b355ff Sep 12, 2019
@gaborcsardi
Copy link
Collaborator Author

@gaborcsardi gaborcsardi commented Sep 12, 2019

And again to fix merge glitches...

@hadley hadley merged commit 7a8da92 into master Sep 12, 2019
10 checks passed
@hadley hadley deleted the feature/rmd-import branch Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants