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

Revisions based on a full read of the blueprints #138

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

# Welcome to this repository!

This repository stores the [__Epiverse TRACE Blueprints__](index.qmd) for software development. You can:
This repository stores the [__Epiverse TRACE-Blueprints__](index.qmd) for software development. You can:

* view the blueprints in [quarto / markdown format](index.qmd) on github

Expand Down
29 changes: 19 additions & 10 deletions code-review.qmd
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
title: Code review
---

The principles and reasoning for code reviews are laid out in the [General principles section](principles.qmd#code-reviews) of the Epiverse-TRACE blueprints. This section explores more of the technical details around conducting a productive code review.
This section explores more of the technical details around conducting a productive code review. The principles and reasoning for code reviews are laid out in the [General principles section](principles.qmd#code-reviews) of the Epiverse-TRACE blueprints.

The ways of working for code reviews within Epiverse-TRACE packages follows many of the guidelines covered in the [Tidyteam code review principles](https://code-review.tidyverse.org/). Instead of repeating all of the information from the Tidyteams documents here we highlight a few areas of similarity between the Tidyteam and Epiverse-TRACE for clarity.

Expand All @@ -17,17 +17,24 @@ The ways of working for code reviews within Epiverse-TRACE packages follows many

One difference to highlight between the [Tidyteam principles](https://code-review.tidyverse.org/) and Epiverse-TRACE is not directly related to code reviews and more to merging strategies. Within Epiverse-TRACE we prefer [rebase and merge](git-branching-merging.qmd#merging-pull-requests-merge-commits-vs-squash-and-merge-vs-rebase-and-merge), to maintain a linear commit history, over the [tidyverse preference of squash and merge](https://code-review.tidyverse.org/author/submitting.html#finishing-a-pr).

Reviewers can be assigned in GitHub. Those assigned should review at their earliest convenience or notify the PR author assigning them that they are unable to review. It is not mandatory for reviewers to be assigned, and reviewing a PR without being assigned can help the PR author complete the merge sooner.
Reviewers are assigned on GitHub. Those assigned should review at their earliest convenience or notify the PR author assigning them that they are unable to review. It is not mandatory for reviewers to be assigned. Reviewing a PR without being assigned can help the PR author complete the merge sooner.

It is left to the maintainer or one of the authors of a package to merge the PR once reviewed. This ensures that code quality is maintained throughout development. The maintainer may be the PR author, in the case of requesting a code review from another member of the team, or may be the PR reviewer, when PR is opened by a contributor. PR authors cannot approve their own PRs even if they are the package maintainer.
It is left to the maintainer or one of the package authors of a package to merge the PR once reviewed. This ensures that code quality is maintained throughout development. The maintainer may be the PR author, in the case of requesting a code review from another member of the team, or may be the PR reviewer, when PR is opened by a contributor. PR authors cannot approve their own PRs even if they are the package maintainer.

Code review can be skipped in cases when the package maintainer or authors makes minimal changes, these include, but are not limited to: not touching user-facing functions, only changing package accessories (e.g. GitHub actions) or minor documentation fixes. In these cases a "Merge without waiting for requirements to be met (bypass branch protections)" option can be ticked and the PR can be merged. For more information on merging see the [Standards for branching and merging](git-branching-merging.qmd).
Code review can be skipped in cases when the package maintainer or authors makes minimal changes. These include, but are not limited to:

- not touching user-facing functions
- only changing package accessories (e.g. GitHub actions) or minor documentation fixes

In these cases a "Merge without waiting for requirements to be met (bypass branch protections)" option can be ticked and the PR can be merged. For more information on merging see the [Standards for branching and merging](git-branching-merging.qmd).

If suggested changes fall outside the scope of the PR, utilise GitHub's feature of generating issues from PR comments. Clicking the ellipsis in the PR comment and selecting "Reference in new issue" will open an issue with reference to the PR.

## Types of review

Utilising GitHub pull requests, we conduct two different types of code reviews, which we will explain in this section. The first, _partial code review_, includes only some of a code base. In other words, this only contains changes to the code from a certain point in the past. The second type, _full package review_, is to facilitate reviewing an entire code base.
Utilising GitHub pull requests, we conduct two different types of code reviews, which we will explain in this section.

The first, [_partial code review_](#partial-review), includes only some of a code base. In other words, this only contains changes to the code from a certain point in the past. The second type, [_full package review_](#full-package-review), facilitates reviewing an entire code base.

### Partial review

Expand Down Expand Up @@ -66,9 +73,11 @@ The full package review setup used by Epiverse-TRACE is inspired by a informativ

:::

The full package review is useful when the entire code base should be shown as changed files. This can be for either the first release of a package, or a subsequent release where all the code can be viewed (useful to understand the package architecture).
The full package review is useful when the entire code base should be shown as changed files. This can be for either the first release of a package, or a subsequent release where all the code can be reviewed (useful to understand the package architecture).

The full package review requires a similar setup process to the version release partial code review.

The full package review requires a similar setup process to the version release partial code review. Instead of a branch from the previous release, we want a branch that is completely empty. This is not possible, but we can create the next best thing, a branch from the first commit using:
Instead of a branch from the previous release, we want a branch that is completely empty. This is not possible, but we can create the next best thing, a branch from the first commit using:

```sh
git rev-list --all | tail -1
Expand All @@ -85,11 +94,11 @@ git push origin review

Now the full package review pull request can be made on GitHub, targeting `empty` from `review`.

Once reviewer comments and suggestions are received, methods for incorporating changes and completing the full package review are outlined in the [next section: Addressing package reviews section](#addressing-package-reviews).
Once reviewer comments and suggestions are received, methods for incorporating changes and completing the full package review are outlined in the [section: Addressing package reviews section](#addressing-package-reviews).

### Addressing package reviews

We have used two strategies for integrating suggestions in package reviews.
We used two strategies for integrating suggestions in package reviews.

1. Commit changes to the `review` branch, once all changes have been made the pull request can be [redirected](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request) to the stable branch and merged.

Expand All @@ -101,7 +110,7 @@ An [example of the first strategy can be found in the **finalsize** R package](h

GitHub's feature of suggestions that can be directly committed within the PR are a useful feature that we recommend using for relatively small changes to the code. In most circumstances, these can be directly committed to the feature branch in the PR. GitHub attributes [co-authorship to those that suggested the change and who commited the change](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request#applying-suggested-changes). This is a nice feature that [enables easy recognition of contributions.](contribution-acknowledgements.qmd)

However, in a scenario where the suggested change cannot be commited to the feature branch in the PR, and instead is incorporated in a different feature branch, the suggestion should still be acknowledged. This is where manually specifying [commit coauthors](https://github.blog/2018-01-29-commit-together-with-co-authors/) can help. When using the suggestions of a collaborator, include two empty lines after the commit message and use the phrase `Co-authored-by:` followed by the collaborator's GitHub name and email (this may be a GitHub no-reply email). See <https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors> for details.
However, in a scenario where the suggested change cannot be commited to the feature branch in the PR, and instead is incorporated in a different feature branch, the suggestion should still be acknowledged. This is detailed on the [Acknowledgements page](./contribution-acknowledgements.qmd#other-ways-to-recognize-contributions)

::: {.callout-tip title="Read more about this principle in application"}

Expand Down
10 changes: 6 additions & 4 deletions contribution-acknowledgements.qmd
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
title: Authorship and Contribution Acknowledgements
title: Authorship and contribution acknowledgements
---

As part of the development process we want to ensure individuals are properly credited for their involvement. This section outlines considerations you should make, and guidelines to follow, in acknowledging collaborators.
Expand All @@ -23,13 +23,15 @@ Roles in `DESCRIPTION` can be chosen from any of those listed in the [MARC Code
> - **ths**: If the package is part of a thesis, use for the thesis advisor.
> - **trl**: If the R code is merely a translation from another language (typically S), use for the translator to R.

We include the ORCID of authors, where available, as part of their inclusion in the `DESCRIPTION`. This helps disambiguate in case of a common name and ensures that authors are properly credited.

## Contributor vs. author distinction

A common question is the difference between **contributors** and **authors**. While there is no strict definition for the level of contribution required to become an author, we should err on the side of generosity and offer authorship when there is hesitancy.
A common question is the difference between **contributors** and **authors**. While there is no strict definition for the level of contribution required to become an author, we err on the side of generosity and offer authorship when there is hesitancy.

As a guideline, consider using the **contributor** role when you anticipate a one-off contribution or limited involvement from an individual. Conversely, use the **author** role when you notice sustained contributions or expect ongoing involvement over time.
As a guideline, use the **contributor** role when you anticipate a one-off contribution or limited involvement from an individual. Conversely, use the **author** role when you notice sustained contributions or expect ongoing involvement over time.

Drawing a parallel with academic publishing: contributors (`"ctb"`) are more akin to people listed in acknowledgements and authors (`"aut"`) are paper authors. The relevance of this parallel is particularly visible in the automatically generated package citation, and the pkgdown sidebar where only authors (`"aut"`) are listed [by default](https://pkgdown.r-lib.org/reference/build_home.html#additional-control-via-yaml)).
If helpful, draw the parallel with academic publishing: contributors (`"ctb"`) are more akin to people listed in acknowledgements and authors (`"aut"`) are paper authors. The relevance of this parallel is particularly visible in the automatically generated package citation, and the pkgdown sidebar where only authors (`"aut"`) are listed [by default](https://pkgdown.r-lib.org/reference/build_home.html#additional-control-via-yaml)).

## Other ways to recognize contributions

Expand Down
Loading
Loading