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

Improve the file created by use_vignette() #445

Closed
rorynolan opened this issue Aug 26, 2018 · 6 comments
Closed

Improve the file created by use_vignette() #445

rorynolan opened this issue Aug 26, 2018 · 6 comments

Comments

@rorynolan
Copy link
Contributor

@rorynolan rorynolan commented Aug 26, 2018

Ordinarily, the output of use_vignette("Cool thing") would be a file with the following YAML header:

---
title: "Vignette Title"
author: "Vignette Author"
date: "`r Sys.Date()`"
output: rmarkdown::html_vignette
vignette: >
  %\VignetteIndexEntry{Vignette Title}
  %\VignetteEngine{knitr::rmarkdown}
  %\VignetteEncoding{UTF-8}
---

whereas it would be great if the vignette title fields and Author fields were filled in too such that it would be

---
title: "Cool thing"
author: "Package authors (from DESCRIPTION file)"
date: "`r Sys.Date()`"
output: rmarkdown::html_vignette
vignette: >
  %\VignetteIndexEntry{Cool thing}
  %\VignetteEngine{knitr::rmarkdown}
  %\VignetteEncoding{UTF-8}
---

I also think it would be good to replace the space in the file name, such that it's Cool-thing.Rmd.

If you agree, I'm happy to implement some or all of this myself with a pull request.

Thanks, I use usethis everyday.
Rory

@jennybc
Copy link
Member

@jennybc jennybc commented Aug 26, 2018

I like the idea re: populating title (and in the same way in both places 🙄). I sure wish the duplication wasn't necessary.

In theory, the tidyverse/r-lib "default house style" is to not have package vignette authors, at least if it just repeats package authors. But, I grant, it's easier to delete a field than add it! Pre-populating with something from DESCRIPTION sounds interesting, but I'm also suspicious the code necessary to do this, in full generality, is worth the maintenance. If you try it, I'd seriously consider using just maintainer, to guarantee it is ONE name. If that's the wrong name, it is easy enough to edit by hand.

I believe the vignette file name is already "asciified"? More specifically, it will not contain a space.

I suspect this comes down to whether populating the title is worth departing from the very simple approach we currently have, which just delegates to rmarkdown::draft().

It's great if you want to explore it, but I think it will be a hard sell if it can't stay very simple and easy to maintain.

@rorynolan
Copy link
Contributor Author

@rorynolan rorynolan commented Aug 26, 2018

Alrighty. It seems that putting in the authors automatically isn't a great idea, wasn't completely sold on it myself anyway. Could maybe change it to INSERT AUTHOR HERE OR DELETE THIS LINE to make it more obvious that that needs to be changed? No biggie either way.

I think changing the Vignette Title bit is doable though. I'd write the output of rmarkdown::draft() to a tempfile, read the tempfile with readLines(), replace Vignette Title appropriately with base::sub(), write the vignette .Rmd file to the appropriate place (the same place usethis puts it now) and then delete the tempfile (I use tempfiles in this scenario so that if something goes wrong, I've only done something temporary to the hard disk).

Then, to make sure this is all working correctly, I'd write a test which should break if there's a change to rmarkdown::draft() which screws up the whole thing?

What do you think?

Also you're right about the file name being asciified, I was using an older version of usethis.

@jennybc
Copy link
Member

@jennybc jennybc commented Aug 26, 2018

What you propose for the title isn't really consistent with usethis practices in general.

You're sort of caught between these two things (which also explains why this hasn't been done yet):

  • We do all templating + data injection with whisker (not with regex, as you describe). We try to read/write DESCRIPTION with the desc package, etc.
  • rmarkdown wants you to use an rmarkdown template for this task, i.e. drafting a document.

I've never looked at all of this hard enough to play it out for the vignette case. Which explains the current state.

@rorynolan
Copy link
Contributor Author

@rorynolan rorynolan commented Aug 26, 2018

Well then, I guess a new vignette template could be put in inst/templates and populated with whisker and then rmarkdown::draft() wouldn't be needed?

Anyway, if you can think of a solution you like, I'm happy to implement it with a PR, if not, I totally understand and won't be at all offended. :-)

@jennybc
Copy link
Member

@jennybc jennybc commented Aug 26, 2018

I think someone just needs to try it and see if it can be done in a nice clean way. I'd be happy to review a PR and hope this provided some direction.

@katrinleinweber
Copy link

@katrinleinweber katrinleinweber commented Apr 29, 2019

Neat improvement, thanks :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants