Skip to content

Contributing to the Code

Jump to bottom
Rida Abou-Haidar edited this page Aug 15, 2019 · 9 revisions

Making a Pull Request

Preamble

As part of the open source community on GitHub, LORIS welcomes any contribution to the code issued by collaborators or any other member of the open source community. The following document is meant to aid in understanding the process of issuing pull requests to the LORIS codebase. Following the guidelines below helps streamline the review process and allows for safer and faster release cycles.

Definitions

GitHub offers several systems for organising Pull Requests and Issues. LORIS uses these systems as defined below.

  • Reviewers: Members selected for code review of the Pull Request in question.
  • Assignees: Members selected for manual testing of the Pull Request in question.
  • Milestones: Target release version for the PR.
  • Labels: Set of descriptors facilitating processing of a PR.
  • Projects: Concern oriented grouping of PRs, issues and notes into a single dashboard representing the current state of the specified project.
  • Base Branches: Descriptors of the nature of the pull request. Bugfix, minor or major.

Procedure

In order for a Pull Request to be processed as quickly as possible, it is crucial for each party involved to fulfill their responsibilities as follows.

Note that only developers who are members of our GitHub loristeam can add labels and milestones to PRs.

The tasks below lie with the Developer issuing the Pull Request:

  1. Choosing the correct branch to issue the Pull Request. See Choosing Branch section below
  2. Provide a clear description for the Pull Request. Following the suggested template is generally the best option.
  3. Adding any necessary related PRs, tickets or context documents.
  4. Selecting appropriate labels. Labels are explained in detail below.
  5. When possible selecting a reviewer. The best reviewers are the module experts (often suggested automatically on GitHub).

The tasks below lie with the Senior Developers and Repository Managers:

  1. Confirm choice of Branch, Reviewers and Labels.
  2. Make suggestions for testers through comments. Different sections of codes can have different independent testers.
  3. Assign the Pull Request to a Milestone to reflect which LORIS release it will be in.

Choosing Branch

Branch selection will depend on the type of changes in the code:

  • Bug Fixes

    • Branch: bugfix
    • Content: Generally these changes do not require SQL scripts and are concise with the sole objective to correct on single problem in the code.

  • Minor Changes and Small Features

    • Branch: minor
    • Content: Features affecting self-contained components such as modules. Additions to Libraries, API or modules that do not change and function signatures.

  • Major Changes, Non Backwards-Compatible Changes and Large Features

    • Branch: major
    • Content: Any change modifying a function signature in a library class. Features require extensive LORIS-wide testing. New complex systems and features spanning across multiple modules and libraries. Deprecated functions clean-up.

Special cases

  • When a Pull Request contains multiple types of changes, it is suggested to divide the PR into multiple smaller ones when possible and issuing them separately; smaller pull requests are easier to review and tend to contain less of a risk of damage to the remainder of the code.

  • In the event that a Pull Request can not be subdivided into smaller ones, it should be categorised in respect to the most impacting change it contains (i.e. a PR containing both bug fixes and minor features would be considered as a minor feature request and would then be issued to the minor branch).

Understanding Labels (Developer and Repository manager)

The following GitHub Labels are designed to be non mutually-exclusive. Proper usage of these options facilitates triage, categorisation and processing of a PR. Every flag added should have an associated section in the main description of the Pull Request justifying the requirement for the aforementioned flag; failure to so can delay the processing of the PR.

Add to Release Notes

  • Description: PR change should be highlighted in Release notes, such as a key security fix, a new dependency, a new practice enforced by the codebase, or other significant change. Not always but sometimes overlaps Feature or Caveat... tag. (Examples)

API

  • Description: PR introduces Modifications to the API code.

Beginner Friendly

  • Description: PR contains changes that can be reviewed or tested by developers new to LORIS. Issue requiring relatively low familiarity with the code to resolve.

Blocked

  • Description 1: PR awaiting the merge, resolution or rejection of an other Pull Request.
  • Description 2: PR flagged for potential rejection.

Bug Fix

  • Description: PR contains bug fixes (not mutually exclusive with the Feature label).

Caveat for Existing Projects

  • Description: PR contains changes that might impact the code or accepted practices of current active projects. Any non backwards-compatible change should have this flag. Any change to library function signatures should have this flag.

Cleanup

  • Description: PR consists of at least one clean-up operation (could be backwards-compatible or not).

Critical to release

  • Description: PR is key for the release to which it has been assigned.

Discussion Required

  • Description: PR awaiting the resolution of a discussion between all involved parties. Discussion should occur on the conversation page of the PR. If discussion is done offline, a short summary is expected to appear on the conversation page.

Document at Release

  • Description: PR adds or changes a feature such that the wiki (or other documentation) must be updated. e.g. new module needs a wiki page, change to install process, imaging pipeline execution changes.

Documentation

  • Description: PR contains modifications to the code documentation including test plans and wikis.

Feature

  • Description: PR implements at least one new feature (not mutually exclusive with the Bug Fix label)

"Help! I don't understand Travis!"

  • Description: TRAVIS disagrees with PR for mysterious reasons.

Meta

  • Description: PR does something that organizes, upgrades, or manages the functionality of the codebase. (Examples)

Needs Rebase

  • Description 1: PR issued to the correct branch but contains conflicts with the current state of the target branch.
  • Description 2: PR issued to the wrong branch.

Needs Work

  • Description: PR awaiting additional changes by the author or contains issues that the author needs to fix.

Passed Manual Tests

  • Description: PR has undergone proper testing by at least one peer.

SQL

  • Description: PR contains SQL modifications such as schema changes and new SQL scripts.

Testing

  • Description: PR contains test plan or automated test code (or config files for Travis)

UI

  • Description: PR alters or improves the current User Interface.

Home | About | Community | Setup | Technical

Clone this wiki locally