-
Notifications
You must be signed in to change notification settings - Fork 12
Dev process
Trevor Bekolay edited this page Sep 4, 2018
·
3 revisions
To facilitate rapid development, we have adopted the following development processes.
Development is driven by pull requests (PRs), which involve people in four roles:
-
Author
- Initially plans and implements an idea (usually based on an existing issue or internal discussion)
- Opens the PR
-
Collaborator
- Gets involved in the PR
- Works closely with the author
- Partnership relationship -- cooperates with the author to complete the PR
-
Reviewer
- Gets involved at the end of the PR
- Arms-length from the author and collaborators
- Critical relationship -- challenges the author and points out flaws
-
Lead
- Ensures that the above roles are filled
- Merges the PR once it has been finalized and the reviewer approves
- May edit PR's commit history as desired
Each PR has one author. A PR can have zero or more collaborators (though having more than three people involved in a PR is likely not productive). Currently, all Nengo Loihi PRs must have at least one reviewer. The project lead is currently @tbekolay. The lead can act as a collaborator or reviewer, but not both.
- Author has an idea they want to work on.
- Author does some planning, maybe completes an initial prototype, convinces themselves that this idea has merit.
- Author opens a PR describing their idea, any initial work, and known TODO items. Author adds the ready for collaboration label.1
- Someone else looking for something to work on sees the author's PR in the ready for collaboration list and decides to collaborate on it.
- Author and collaborator sync up. In most cases, this should be done through a high-bandwidth medium like a video call or in-person meeting.
- Author and collaborator decide how to proceed, continue working on the project together while maintaining tight communication.
- Once author and collaborator believe the PR is complete, they add a comment to the PR stating what has been done and anything reviewers should know, then remove the "ready for collaboration" label and add the "ready for review" label.
- If the author and collaborator think that a certain person would be a good reviewer, they request a review from that person when they add the "ready for review" label. When requesting a review from someone, also mark that person as "assigned" to the PR.
- If not, anyone is free to request a review from themselves and assign themselves, which explicitly volunteers to act as the reviewer.
- If a reviewer has not been identified within a reasonable amount of time, lead will choose a reviewer.
- If the chosen reviewer is not available or not confident they can be a good reviewer, they choose someone else to act as the reviewer.
- Reviewer looks over the PR, makes comments and suggestions with Github's inline code comments.2
- If reviewer thinks there is significant work to be done, the PR is re-labeled as "ready for collaboration." If the reviewer is able, they can at this point become a collaborator and implement their own suggestions. In either case, collaborators should go back to step 6.
- Once reviewer is satisfied, they give the PR an "Approved" review through the Github code review interface and remove the "ready for review" label.
- Lead will modify branch history if appropriate, being sure to acknowledge all collaborators in commit messages, and then merge the PR.
Notes:
- The author should continue working on the PR at this point; marking a PR as "ready for collaboration" is not blocking.
- The author and collaborator need to implement the reviewer's suggestions or convince them to change their mind. The onus here is on the collaborators as the reviewer should not approve a PR if they have unanswered questions.
For a small fix, the above workflow can happen rapidly so as not to bog down PRs in unnecessary process.
- Author identifies a bug and implements a fix.
- Author opens a PR for the bugfix and adds the "ready for review" label.
- Lead reviews the PR, marking it as approved.
- Lead merges the PR.
In this relatively common case, only two people needed to be involved and require minimal interaction to make progress. It should be noted that in this case, both the author and lead are expected to run the unit tests.
For an average sized feature, the above workflow can move at a steady pace.
- Author chooses a feature to be implemented from the issues list.
- Author works on it for an afternoon, making progress but not completing the feature.
- Author makes a PR with their work in progress and their TODO items. They add the "ready for collaboration" label.
- Author continues working on the feature. People see their work in progress and add a few comments with suggestions, but do not commit to being a collaborator.
- Author finishes the TODO items to their satisfaction and marks the PR as being ready for review.
- One of the commenters reviews the PR, leaving inline comments on a few lines that could be simplified, a function that could use a test, and a class that needs a docstring. Reviewer marks the PR as needing changes, removes the "ready for review" label, but does not add the "ready for collaboration" label because the changes should not take long to implement.
- Author makes the changes and responds to each of the inline comments stating what they changed.
- Reviewer confirms that those changes are satisfactory, and gives the PR an approved review.
- Lead confirms that the unit tests still pass, fixes up the fixup commits introduced through the review process, and merges the PR.
In this case, three people were involved (author, reviewer, and lead). Most of the work was done by the author, but the reviewer and lead also now know how that feature works.
Changes that touch a large part of the codebase can take several back-and-forths, and should involve several people.
- Author has an idea for how to better structure the code to make it more readable and testable.
- Author works on it for an afternoon, making some slight progress, but it's not possible to run anything as the code is mid-refactor.
- Author makes a PR for it anyhow, explaining the idea behind the refactoring, and noting the TODO items yet to do. They add the "work in progress" label as the PR is not yet ready for collaboration, but they want to make the team aware that of what they're working on.
- Author works on it for another few days and gets to a point that the code runs, but there are still remaining TODO items. Author removes the "work in progress" label and adds the "ready for collaboration" label.
- Someone notices that a TODO item touches a piece of code they are familiar with. They leave a comment in the PR saying that they would like to refactor that part of the code, and therefore become a collaborator.
- Author responds to the collaborator in the PR, then contacts them privately to set up a video call in which author explains what they've done so far and what they think should happen with the piece of code the collaborator wants to work on.
- Author continues working on other TODO items while collaborator works on their piece of code.
- As author and collaborator make progress they push commits and let each other know when a commit is pushed so that the other can look it over.
- After some time, both author and collaborator are satisfied with the state of the PR, so they mark it as "ready for review."
- Since it is a big change that no one wants to review, lead requests a review from a specific team member who has extra cycles.
- Reviewer is unfamiliar with some aspects of the code, so makes many inline comments asking questions, some related to the refactoring and some unrelated.
- Reviewer marks the PR as needing changes and marks it as "ready for collaboration" as there are many aspects of the code that are unclear even post refactoring.
- Author and collaborator use the questions to refactor further, and add additional tests and documentation. Some of the inline comments made by the reviewer are important, but tangential to the point of the PR, so they make separate issues to ensure that the comments are acted upon later but do not block the PR.
- Author and collaborator respond to each inline comment with how it was resolved, and mark the PR as "ready for review."
- Reviewer accepts most of the resolutions, but with their new understanding of several parts of the code sees a few improvements that could be implemented quickly. They remove the "ready for review" label, but mark the PR as needing changes.
- Author implements those remaining issues and responds to those inline comments.
- Reviewer is mostly satisfied with the new changes, and so approves the PR. They make a mental note of some minor things that could be improved in the future, but aren't important enough to warrant an additional round of review.
- Lead reads through the history of the PR in order to ensure that the commit history makes sense given the original intent of the author. They ask the author a few questions in a private IM to ensure that commits are attributed to the right person, or have the right co-authorship information.
- Lead checks with author and collaborator to ensure that the history is satisfactory, then merges the PR.
In all, four people were involved in the PR, which for Nengo Loihi is most of the dev team. Everyone contributed in some way, and everyone involved should have a better understanding of the code. Additionally, since they were involved in the PR, they may wait to work on certain parts of the code because it will be easier to work on post-refactoring.