Pull Request Review Memo

You were asked to review a PR? Here’s what you should keep in mind:

By default we don’t do in-depth review of everything unless it was specifically requested. Sometimes PRs are just too big to review everything, and more often than not it would’ve been a waste of time. So it’s a responsibility of the person who asks for a review to clearly specify what needs to be carefully reviewed, unless the scope of the PR is very limited and the subject of review is crystal clear.

Although it should be said that for the PRs with package updates it is highly recommended, and we usually do request detailed code audit. In addition to that the “not in-depth” review of package PRs already implies a substantial set of checks. More on this in the dedicated chapter.

At a minimum we do a surface check for “common sense”. It’s hard to formalize all the nuances of the so-called common sense, but here are the main points:

1. First of all PRs must not violate our Hygiene of directories structure guidelines. So please check that the PR does not accidentally contain any files that are not meant to be committed. This includes output files for which there’s a source and temporary files of different kinds. If the PR makes some updates to gitignores please check that it follows the mentioned guidelines.

2. PRs stay inside the declared scope. If, for example, it’s a project-related PR it shouldn’t accidentally touch files outside of the project. If it does, there should be an explanation. And it’s not like you should demand explanations for each and every file, just be more critical when you see such things. One more comment with regards to the scope. Several times we tried an approach of developing a package update and using this update on a project within one PR. It almost never turns out to be the best idea and quite often it creates additional hurdles when it’s time to merge the PR. If it’s the case, please discuss with the PR creator if it would be easier to separate the two things.

Below you’ll find additional comments for the two main types of Pull Requests, project and package PRs.

There’s not a lot to say on top of the general recommendations. We initialize projects with templates (g6tr_setup() / g6tr_newsletter_setup()), and we have a set of default git ignore rules so it’s hard to mess up the file structure. It should be easy to notice when something is very off. You are not required to check the code in the project vignettes unless you were specifically asked.

Package updates require special attention because this is what’s going to be reused in other packages or projects. Here’s the bare minimum of checks:

1. R CMD check. It is executed automatically by github when you mark the PR as “ready for review”. So you should only proceed with other checks when this build is successful. If it’s not successful, the person who asked for the review is responsible for fixing it.

2. Version is updated in DESCRIPTION and NEWS.md, news are properly filled in.

3. All the functions or vignettes have been added to the package’s pkgdown/_pkgdown.yml file. Our GH builds don’t automatically check for this.

4. All exported functions have properly formatted grammatically correct documentation. Unless the function is trivial or it does not have arguments, it should have at least one example. Complicated internal functions should have at least minimal roxygen documentation with descriptive @title and/or @description. If internal function does not have any parameter documentation that is inherited by any other function, it should be marked with @noRd. This tells roxygen to ignore the function. Otherwise the function should be marked with @keywords internal. This makes pkgdown ignore the function.

5. Carefully review examples of the new exported functions. R CMD check does ensure that examples can be run, but you have to check that they all make sense and actually help with understanding how the function operates.