CreatePackageVignette function

[jayqi]

Note: This PR is branched off of #181

New function CreatePackageVignette creates an html_vignette version of the package report (long form, no tabs). This makes it easy for any package author to include a pkgnet report as a standard knitr-buildable vignette for their package. It will even render natively inside pkgdown websites.

pkgdown Demo: https://jayqi.github.io/pkgnet/docs/articles/pkgnet_report.html

[codecov-io]

Codecov Report

Merging #200 into master will increase coverage by 0.67%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #200      +/-   ##
=========================================
+ Coverage   91.52%   92.2%   +0.67%     
=========================================
  Files          11      12       +1     
  Lines         920    1000      +80     
=========================================
+ Hits          842     922      +80     
  Misses         78      78

Impacted Files	Coverage Δ
R/AbstractGraphReporter.R	86.36% <100%> (+0.07%)	⬆️
R/AbstractPackageReporter.R	100% <100%> (ø)	⬆️
R/CreatePackageVignette.R	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71d7fa8...a1e96ac. Read the comment docs.

[bburns632]

This is a very slick function! I have a few suggestions to reduce user error I'd like us to discuss.

[jameslamb]

This is great Jay! I love that we're at the point where we can work on these "make pkgnet more fun for people" stage in development. This type of contribution is incredibly important.

I basically approve, but I would like to review one more time once you rebase to master

[jayqi]

@bburns632 @jameslamb So I ran into a problem with my implementation, and I'd like your input on how to resolve.

Basically, because this is generating an .Rmd file, it's essentially lazily evaluated at package build time.

This means that if you want to provide pkg_path (for test coverage in FunctionReporter), it has to be the path that will work at build time. You can't do an absolute path because Travis will likely have a different absolute path, and it would break your Travis build. (Travis builds will try to build your package and therefore build your vignettes.) So, since in general R CMD build seem to use the package root as the working directory (no matter where you executed it from), it has to be the case that pkg_path = ".". So it really doesn't make sense to offer pkg_path as a parameter.

So I see three options here:

1. Use a logical argument for whether or not to pass pkg_path to the reporters in the generated vignette. It's not clear to me what to name or communicate this argument. It's not required for FunctionReporter to be one of the reporters.

2. Just hard-code to always pass in pkg_path = ".". In principle, it should always work because generally you're only building the vignette as part of the source code of your package. This means people will get test coverage whether they want it or not.

3. Just don't support the ability to have test coverage for the vignette. We'll figure out how to add it back in later.

For now, I'm going to go with (3) but I'm pretty torn about how to do this. I think this really tells us we should figure out how to make the interface more clear.

[jameslamb]

This is great Jay! Sorry it took so long to review. Most of my suggestions are minor internal things, but curious to hear what you think about my proposal to make this behave like devtools::document()

[jameslamb]

alright I've been thinking about this...I have an idea we could use to simplify this a bit.

Since the intended use is "run this interactively when you already have the source of the package sitting on your filesystem", I think we should use the same interface as devtools::test(), devtools::document(), and devtools::check() (tools that people are VERY familiar with already). This would look like:

CreatePackageVignette(
    pkg = ".",
    pkg_reporters = whatever,
    vignette_path = whatever
)

Benefits this would have:

1. No new complexity (since we are already parsing the DESCRIPTION file with read.dcf(), we can just get pkg_name from there)
2. Consistency with existing tools in the R developer ecosystem
3. Default call becomes CreatePackageVignette() with no args and "just works"
4. Cut some of the "try to figure out what folder you're in" complexity from the internals

@bburns632 @jayqi what do you think?

[jayqi]

I agree that would help simplify the interface. A few potential hiccups I see are:

1. Do we force the vignette report to always include pkg_path and calculate test coverage?
2. What if a user did want to create this vignette format without having the source code? Having the source code is not strictly necessarily under my original interface.
3. Do we worry about the source version diverging from the loaded version? I believe devtools loads the source code for those functions. Do we need to load it too to ensure that there is consistency? FunctionReporter generally has this problem too in other usages, but I feel like we will fail the principle of least astonishment if we don't load the source code, if the main entrypoint is that a user is pointing the function at their package root.

[jameslamb]

I don't understand how #1 is related to my recommendation.

I'm struggling to come up with a realistic use for "I want to make a vignette but I don't have the package source code". Why would someone who doesn't have the package source code care about making an R vignette instead of just running CreatePackageReport()?

3 is something I hadn't thought of, fair point. I think as long as we document the fact that you have to do devtools::install() or install.packages() or R CMD INSTALL before running this, it's ok. You have to have installed the package already in your current interface, so while I get that it's a concern I don't think it's one that informs whether or not to go with my suggestion.

In every R package I've ever written, I always have an order of steps like this in CI or publishing:

1. devtools::install()
2. devtools::document()
3. R CMD CHECK (also running unit tests`

So I think the "you have to install first" is a tolerable divergence from what devtools::test() etc. do anyway.

I also want to reiterate that the "simplify the interface" benefit of what I'm proposing is nice, but my main reason for proposing this is the "consistency with all the devtools stuff" reason. I think that is a nice way for us to integrate with normal R developer workflows.

[jayqi]

Okay @jameslamb, I think what you said all makes sense. I agree that overall the balance is we benefit from having it work with a devtools-style interface.

Is it confusing if this interface is different than CreatePackageReport? Should we name this function something else to make it clear not to expect a similar interface?

[jameslamb]

I don't think it's confusing to have a different interface for this function

[bburns632]

This whole function is tailored for a very specific scenario. I would not think the above is all that confusing, especially since I imagine 90% of the uses will be without parameters.

[jameslamb]

^ agree with @bburns632

[jameslamb]

I just re-reviewed. Looking great! I still feel strongly about making the interface look like devtools::stuff() and, in doing that, committing to the fact that this function is intended to be used by package contributors who have the package source code available locally for development.

[jameslamb]

I don't understand how #1 is related to my recommendation.

I'm struggling to come up with a realistic use for "I want to make a vignette but I don't have the package source code". Why would someone who doesn't have the package source code care about making an R vignette instead of just running CreatePackageReport()?

3 is something I hadn't thought of, fair point. I think as long as we document the fact that you have to do devtools::install() or install.packages() or R CMD INSTALL before running this, it's ok. You have to have installed the package already in your current interface, so while I get that it's a concern I don't think it's one that informs whether or not to go with my suggestion.

In every R package I've ever written, I always have an order of steps like this in CI or publishing:

1. devtools::install()
2. devtools::document()
3. R CMD CHECK (also running unit tests`

So I think the "you have to install first" is a tolerable divergence from what devtools::test() etc. do anyway.

I also want to reiterate that the "simplify the interface" benefit of what I'm proposing is nice, but my main reason for proposing this is the "consistency with all the devtools stuff" reason. I think that is a nice way for us to integrate with normal R developer workflows.

[bburns632]

I defer to whatever consensus you two reach on the devtools::stuff() like interface for this function.

[jameslamb]

I enthusiastically approve. Great work, Jay!

I promise I'll do this for uptasticsearch