- Uphold the design principles and package mechanics outlined below.
- When in doubt, discuss in an issue before doing lots of work.
- Make sure the package still passes
R CMD check
locally for you. It’s a good idea to do that before you touch anything, so you have a baseline. - Match the existing code style. Please use the styler package to re-style any code that you touch.
- Tests: please try to run our tests or at least those that exercise your PR. Add tests, if relevant. If things go sideways, just say so. We are painfully aware that it’s not easy to test API-wrapping, auth-requiring packages and are open to constructive feedback. More below.
- Documentation: Update the documentation source, if your PR changes
any behavior. We use
roxygen2, so you must
edit the roxygen comments above the function; never edit
NAMESPACE
or.Rd
files by hand. More below. - Website: The pkgdown-created website is built and deployed
automatically via Travis-CI. Some changes require an edit to the
reference
section of_pkgdown.yml
, i.e. to make sure that a function appears there. - If the PR is related to an issue, link to it in the description,
with the
#15
syntax and the issue slug for context. If the PR is meant to close an issue, make sure one of the commit messages includes text likecloses #44
orfixes #101
. Provide the issue number and slug in the description, even if the issue is mentioned in the title, because auto-linking does not work in the PR title.- GOOD PR title: “Obtain user’s intent via mind-reading; fixes #86”.
- BAD PR title: “Fixes #1043”. Please remind us all what issue #1043 is about!
- BAD PR title: “Something about #345”. This will not actually close issue #345 upon merging. Use the magic words.
- Add a bullet to
NEWS.md
with a concise description of the change, if it’s something a user would want to know when updating the package. dplyr’sNEWS.md
is a good source of examples. Note the sentence format, the inclusion of GitHub username, and links to relevant issue(s)/PR(s). We will handle any organization into sub-sections just prior to a release. What merits a bullet?- Fixing a typo in the docs does not, but it is still awesome and deeply appreciated.
- Fixing a bug or adding a new feature is bullet-worthy.
- When in doubt, take a cue from the Unix file system commands.
- Have a reasonable default whenever humanly possible. This applies to auth, file name, file location, etc.
- Be pipe-friendly.
- If it’s not well-documented (e.g. working example!), it doesn’t really exist.
- Accommodate initial file specification via path or name, but constantly push downstream work to be based on file id.
- Return a tidy tibble, almost always a
dribble
, whenever it makes sense.
There is a high-level interface for the typical user. These functions help you accomplish the most common tasks, hopefully in a natural way.
We use roxygen2,
specifically with the Markdown
syntax,
to create NAMESPACE
and all .Rd
files. All edits to documentation
should be done in roxygen comments above the associated function or
object.
Use templates or inheritance to repeat documentation whenever it is helpful, but without actually repeating its source.
Use internal and external links liberally, i.e. to other docs or to NYSDOH/IPRO resources.
We encourage working examples that include any necessary setup and
teardown. In most cases, you’ll have to put them inside a \dontrun{}
.
It’s nice if a pull request includes the result of running
devtools::document()
, to update NAMESPACE
and the .Rd
files, but
that’s optional. A good reason to NOT document()
is if you have a
different version of roxygen2 installed and that sprays minor formatting
changes across .Rd
files that have nothing to do with the PR.
We use testthat.
We have many tests that rely on the existence of specific files and folders. Therefore, to fully test nysdoh_sepsisr, you have to do some setup.
For small changes, it’s fine to test your specific change locally and make a PR. Keep reading for an explanation of how to run full tests for nysdoh_sepsisr.
For speed reasons, the nysdoh_sepsisr tests expect to find certain pre-existing files and folders, i.e. we don’t do full setup and tear down on each run. You do setup at the beginning of your nysdoh_sepsisr development and leave these files in place while you work.
-
Source
tests/testthat/driver.R
to extract and aggregate the current setup and clean code across all test files.## gather all the test setup and clean code from individual test files source(testthat::test_path("driver.R")) ## leaves behind: ## * all-test-setup.R ## * all-test-clean.R
- This creates two R scripts:
tests/testthat/all-test-setup.R
andtests/testthat/all-test-clean.R
. Inspect them.
- This creates two R scripts:
-
When you are truly ready to perform setup or clean, edit the code to set the
SETUP
orCLEAN
variable toTRUE
instead ofFALSE
. This friction is intentional, so you don’t accidentally create or delete lots of Sepsis files without meaning to. -
Render
all-test-setup.R
with the Knit button in RStudio or like so:
rmarkdown::render(testthat::test_path("all-test-setup.R"))
You could also just source it, but it’s nice to have a report that records what actually happened.
You should now be able to run the tests via Build > Test Package or
Build > Check Package in RStudio or via devtools::test()
.
You can leave the setup in place for as long as you’re working on nysdoh_sepsisr, i.e. you don’t need to do this for every test run. In fact, that is the whole point!
When your nysdoh_sepsisr development is over, render the clean script:
rmarkdown::render(testthat::test_path("all-test-clean.R"))
Again, read the report to look over what happened, in case anything was trashed that should not have been (btw, let us know about that so we can fix!). Once you’re satisfied that your own files were not touched, you can truly delete the test files.
If you’re going to add or modify tests, follow these conventions:
- Test files are marked up with knitr chunk headers in comments,
e.g.
# ---- clean ----
or# ---- tests ----
. This is what enables thedriver.R
script to isolate the setup or cleaning code. Don’t break that. - Any file that is truly necessary and can be setup in advance and persist? Do it, in order to make future test runs faster. Put the associated setup and clean code at the top of the test file.
- All test files should have a name that documents why they exist and
who made them. Use the
# ---- nm_fun ----
chunk to define naming functions used in that test file (see existing files for examples). Always use one of these functions to generate file names. Usenm_()
for test files that persist. Useme_()
for ephemeral test files that are created and destroyed in one test run.
Example and structure of a self-documenting name for a persistent test file:
move-files-into-me-TEST-drive-mv
<informative-slug>-TEST-<test-context>
Example and structure of a self-documenting name for an ephemeral test file:
DESCRIPTION-TEST-drive-upload-travis
<informative-slug>-TEST-<test-context>-<user>
Note that the current user is appended! This is so that concurrent test runs do not attempt to edit the same files.
nysdoh_sepsisr is checked on a large matrix of R versions and operating
systems via GitHub Actions. In general, the package is subjected to
R CMD check
, unit tests, and test coverage analysis after every push
to GitHub.
Things are a bit different for pull requests from outside contributors,
however. These PRs do not have access to the encrypted tokens, therefore
many tests must be skipped. The PR will still be vetted via
R CMD check
and tests that do not call Sepsis data can still be run.
After you make a PR, it’s a good idea to check back after a few minutes
to see all of these results. If there are problems, read the log and try
to correct the problem proactively. We “squash and merge” most pull
requests, internal or external, so don’t agonize over the commit
history.
Please note that the nysdoh_sepsisr project is released with a Contributor Code of Conduct. By contributing to this project you agree to abide by its terms.