Working With Pull Requests¶
Documented here is the procedure for working with pull requests (PRs)
in the dbprocessing project. It does not include details on working
with git and github in general; relevant docs are linked.
If you can’t figure it out, please do your best and ask for help in the PR
(use the question tag).
Creating a PR¶
It is strongly recommended to create a pull request from a topic
branch (not master) in a fork of the spacepy/dbprocessing
repository. This allows a
clean separation of code that’s in-progress vs. ready-to-go, and also
a distinction between different pull requests. Submitting a pull
request from master, even of your fork, makes it difficult to have two
open PRs, and make cleanup after a PR is merged very difficult.
Before working on an issue, then, fork the spacepy/dbprocessing
repository. Make a branch. Work on the branch locally and, when complete, push
the branch to your fork. Then open a pull
request
against spacepy/dbprocessing master.
If you use CircleCI, unfollow your fork before submitting a PR.
The preferred flow of code is summarized:
Code is created on a branch of a fork, not directly on master.
Code enters the
dbprocessingrepository via pull requests. This applies to contributors and developers alike; developers do not push directly. (Usually PRs are relative to themasterbranch but in some cases a topic branch may be created.)Code enters the
masterbranch of a fork by syncing upstream to the fork after the pull request has been merged.
When creating the PR, following the provided template as closely as possible will facilitate its review.
Use closes #x in the description of the PR if merging the PR will
close that issue. (Closes is preferred to fixes because e.g
closing an enhancement issue is not exactly a fix.) Referencing
other related issues or PRs is also encouraged, e.g. see #x.
Avoid the issue-closing magic words unless closing the issue,
in which case closes is preferred.
The template includes a checklist; consider every item on the list and check it if completed. If an item is not relevant, check it, add “(N/A)” to the start of the line, and include an explanation below the checklist. E.g.:
- [X] ...
- [X] (N/A) Major new functionality has appropriate Sphinx documentation
- [X] ...
This is a pure bugfix, no new functionality or documentation.
If working in a draft PR, adding more checklists to the description is fine. A PR will be reviewed when:
All checklists are checked (this shows in the pull request list as e.g. “8 of 8”).
The PR is marked ready for review, i.e. not draft.
All CI checks pass.
Feel free to request help before this point (tag the PR with question
to make it stand out).
Reviews and updating¶
Developers will review PRs for inclusion, but reviews and comments are welcome from all. Our experience has been that using the github interface to suggest line-by-line diffs doesn’t work very well; line-by-line comments are fine (and helpful!)
You can request a specific reviewer, but are not required to.
The review will mostly evaluate whether the PR checklist has been met, all tests pass, and the contribution meets requirements in the Developer Information.
If changes are requested, they can usually be addressed by making additional commits on your branch and pushing to your fork. The PR will be automatically updated.
In some cases the master branch of the repository may have changed in a way that’s incompatible with the changes in the pull request. The solution is to rebase the topic branch against the new master. (The easiest way to do this is to update the fork master from the upstream master, then rebase the branch.) In the case of a conflict rebase, this can get messy…feel free to ask for help. A developer may be able to perform the rebase if maintainer edits are enabled. After the rebase, the updated branch will have to be force-pushed to the fork on github.
Conditions for merging¶
In order to be merged, a pull request must:
Have an approving review from a developer who is not the author of the PR. (Usually PR authors are barred from reviewing their own work.)
Have no outstanding requests for changes from developers. Change requests can be addressed either by updating the code or convincing the developer through the discussion that the requested change is not desirable. Outstanding change requests from contributors who are not developers should also be seriously considered; it is preferred to have them resolved.
Be open for at least 72 hours, so all developers have a chance to review. A major change (by discretion of developers commenting) should remain open for 168 hours (i.e. one week) if at all possible; urgent changes (again, by discretion of developers) may be merged 24 hours from submission.
Pass all continuous integration checks, unless there is a documented failure in CI and the PR is part of fixing that failure.
Be merged by someone who is not the author of the PR. Developers may not merge their own PR, even with an approving review.
Be merged by someone who has made an approving review.
Be merged by someone who has not contributed any code to the PR. There are two exceptions, provided all other requirements are met:
A PR may be merged by a developer whose only contribution is to rebase the code to account for changes in master.
A PR authored by a developer may be merged by a different developer who has authored commits that make relatively minor changes to the PR; again, at the discretion of the developers involved.
Developers pledge to make an effort to review pull requests within one week.
Pull requests are merged via the rebase and merge method. This maintains a linear history and also makes it clear both who authored the commit and who approved it for the repository.
Once all conditions are met, a developer can perform the merge.
Post-merge cleanup¶
After merge, the contents of the pull request are in two separate sets of commits: the original commits on the topic branch, and new commits on master. To finish cleanup, the fork should be synchronized to the updated master and the topic branch deleted.
Release: 0.1.0 Doc generation date: Feb 10, 2022