CTU-Bern / presize

Precision Based Sample Size Calculation
https://ctu-bern.github.io/presize/
GNU General Public License v3.0
15 stars 13 forks source link

Improve documentation #66

Closed TomKellyGenetics closed 3 years ago

TomKellyGenetics commented 3 years ago

Review: documentation

Part of review for JOSS.

Examples

The following functions do not have an @examples or \examples field in their "man" or R files. Please add these so they are displayed in the R/RStudio help system.

Ideally several examples should be given, e.g., for Pearson and Spearman correlation and all examples should be well commented to explain the context for understanding use cases.

~- [ ] usage for parameters without defaults explained in examples~

Shiny App

The documentation for the shiny app needs more details. It is not clear before launching what the app does.

Building dev version of documentation

Documentation is not available for the dev version 0.2.1 installed from GitHub. These comments above are based on CRAN release 0.1.3 because of this, although the GitHub source seems to have missing examples also. Building the docs with devtools or roxygen2 before pushing to GitHub is recommended (it is required for uploading to CRAN)

Suggestions

The review does not hinge on these but are recommended for benefit of users understanding the software.

Optional:

aghaynes commented 3 years ago

Thanks @TomKellyGenetics.

I have added examples for the three functions you mentioned and added extra info and the screenshot to the launch_shiny_app help file. I've also added a presize-package helpfile and the app screenshot to the readme (as per your suggestions).

A quick question regarding the examples... you say "usage for parameters without defaults explained in examples"... as the examples are on the help pages and the help pages already explain the parameters, isn't this a bit over the top? Examples in most helpfiles are very minimal...

I'm actually quite surprised that the helpfiles aren't there... I can see them when I install through remotes... In general, one of our github actions actually builds all our package documentation for us (including the pkgdown site) each time something is merged in to the main branch, so everything should be up to date.... although this might not be the case for 0.2.2 on my fork...

Regarding your vignette suggestion, it would indeed be nice, but to do it well would require a little bit of time I think. Something to think about for the (near) future... is that OK?

TomKellyGenetics commented 3 years ago

Installing remotes::install_github("CTU-Bern/presize") installs presize version 0.2.1 and remotes::install_github("aghaynes/presize") installs versions 0.2.2. In both cases the documentation is now available in R. 👍

Is the pgkdown and devtools::document() workflow triggered when merging to CTU-Bern/presize? I see the examples are updated on your branch with comments in the code. Presuming this is merged into the package, this is acceptable.

I'd prefer more explanation of the examples such as how these apply to real-world data or study design. A vignette would be the best place for long-form documentation like this. I understand it would take more time. As examples are also explained well in the README and manuscript, this is not a strict requirement for publication.

aghaynes commented 3 years ago

It is odd that for you, installing via remotes::install_github("CTU-Bern/presize") installs v0.2.1... I get 0.2.2, as it should be...

> remotes::install_github("ctu-bern/presize")
Downloading GitHub repo ctu-bern/presize@HEAD
[truncated for brevity...]
** testing if installed package keeps a record of temporary installation path
* DONE (presize)
> packageVersion("presize")
[1] ‘0.2.2’

Yes, pkgdown and devtools::document are both triggered automatically on a merge (technically a push, but that's the same thing in a PR). It's handled by this action.

Re the vignette, can we consider the issue resolved if we agree to include a vignette in the next version (0.2.3 presumably, assuming CRAN accept the current version as is) of the package?

One question regarding html vignettes (as I notice that you have a few on at least one of your packages), if I may... in another package, we tried to include a html vignette and got a message back from CRAN that you have to indicate external dependencies (such as pandoc; they even set the package as to-be-archived, so we had to reformat everything for PDF vignettes). I've looked at other packages with html vignettes and could never determine where that is done... googling has also never helped me. Can you shed some light on that for me?

TomKellyGenetics commented 3 years ago

You're correct, installing from GItHub now gives the correct version:

> packageVersion("presize")
[1] '0.2.2'

A possible explanation for this is that I cloned the repository and run devtools locally to build the documentation. I may have been using an outdated version last week if the package was updated in the meantime.

I'm okay for version 0.2.2 to be published without a vignette, although I would encourage it for the next release. For HTML vignettes you have 2 options.

  1. Provide vignettes in Rmarkdown. They will be automatically built as HTML on CRAN the header of the Rmarkdown file will tell CRAN which Vignette builder to use. See this example. You can check vignette building with devtools.

  2. You can provide pre-generated static HTML vignettes with some modifications these are CRAN compatible.

The current docs are acceptable to me, although I understand some changes may be needed to pass CRAN checks.

aghaynes commented 3 years ago

Great, thanks for the info @TomKellyGenetics :)

aghaynes commented 3 years ago

@TomKellyGenetics, I guess that this issue can be closed?

TomKellyGenetics commented 3 years ago

If you have more questions about the vignettes, please let me know! Otherwise the issues raised here have been addressed. I've installed the updated package from GitHub and verified that the documentation including examples is available in the R command line. 👍

aghaynes commented 3 years ago

Thanks @TomKellyGenetics! :)