GitHub Guidelines For Collaborative Coding

Posted on February 10th, 2023 by Runtime Verification
Posted in Software Development

GitHub Guidelines For Collaborative Coding

At Runtime Verification, we want to be as efficient as possible with our software development, but working on a team necessarily introduces inefficiency, which companies must manage. Being remote-first also presents unique challenges that teams must actively and intentionally overcome.

An example of why efficiency matters: suppose you have a six-week engagement with a team of two (12 person-weeks), and there are three separate half-day delays per week (seems small, right?). Then you have a minimum of 9 days delay (1.8 weeks, 15%). Delivery could have happened roughly a week earlier. Likely these delays even trigger other delays compounding the issue (delay in one timezone causes blocked work in another timezone?).

These guidelines are ones that RV uses to manage group coding and may help your team avoid introducing unneeded friction into the group coding process. They are guidelines, which means one should strive to follow them but understand that real life is often unpredictable. Therefore such procedures may need to be adjusted to accommodate the situation.

In the ideal development scenario, everyone would be merging at least one pull request (PR) daily for each project they are working on so everyone is working with the most up-to-date code possible. Merging a PR requires three main steps: (i) write code, (ii) pass CI, and (iii) pass review. Each part of this process takes time, and step (iii) requires synchronization with someone else. The overall process is just beginning when you are writing the code.

The best way to ensure low friction is to open small “obviously correct” PRs. In the smoothest scenario, you open a PR that is small and focused and makes an improvement. The reviewer can quickly tell it’s a good thing and approve/merge it before asking you for more feedback. In this scenario, the burden of creating “easy-to-merge” PRs is on the PR author. When the PR author does not make “easy-to-review” PRs, several things happen: (i) the change description cannot adequately describe the change set, thus we lose track of what changes are made, (ii) review quality goes down drastically, and (iii) the PR stalls out in the review for a while (meanwhile the code is changing, which leads to merge conflicts, and other team members are not getting any benefit from the PR yet).

Prioritizing Work:

Ideally, the developer would handle the to-do list in this order:

  1. Review any PRs asking for your review and passing CI, or coordinate with other reviewers to assign it to them. Failing to ensure that PRs are being reviewed as requested also blocks the PR author.
  2. Any open PRs that have reviews on them need to be addressed.
  3. Any of your draft PRs need to get as close to being ready for merge as possible (feature-complete, passing tests, etc.).
  4. Anything else (picking up new tasks, explorations, experiments, etc.).

When following these steps, the time zones of the people you work with should be considered. If you have 2 PRs open that you need to review or that you need to fix, (a) one for someone who is working now and will go offline soon, and (b) one for someone who is not yet online anyway, prioritize working on PR (a) over PR (b) in your queue.

Opening PRs:

  • Make small, focused PRs that address precisely the problem that is needed.

  • Keep your git history clean and organized, with isolated changes introduced in each commit and well-tailored commit messages. Ideally, each commit should build on its own (compile) and, even better (though not always possible), should pass tests on its own. Keeping a clean git history makes every other step in the process half as difficult.

  • Do not have a mix of PR “types.” Different types of PRs are (i) bug fixes, (ii) new features, (iii) refactors, and (iv) code reorganizations. For example, you should not have a bug fix and a code reorganization in the same PR. This rule has many exceptions, and programming is not always done in a straight line. We often do encourage developers to make small cleanups and refactorings alongside their other changes, but following two rules: (i) anything sweeping or that grows in size moves to its own PR, and (ii) those commits are isolated from the rest of the changes, so it’s easy to pull them out if the PR reviewer asks for it.

  • When you open the PR, open it as a Draft, and ensure it gets tracked by the project management tools. To achieve this, you must assign yourself and then either:

    • Add it directly to the appropriate project board on GitHub, or
    • Mark it as fixing an issue that is attached to a project board.

  • When you open a PR, do a self-review on GitHub. Doing so will encourage you to look over the code the same way you expect someone else to. It also enables you to look over the code using a different presentation than the editor you are used to. Things you should check for:

    • Are there any code changes that were added unintentionally? If so, remove them. Examples include comments, log files, temporary inputs, print/debug statements, etc.
    • Was there scope creep? Should anything be removed that is separate from the main focus of the PR?
    • Are all the significant changes succinctly summarized in the main issue body of the PR?
    • Have you written tests of any new functionality or tests demonstrating the bug this PR fixes?
    • Is there any documentation that should be updated along with this PR?
    • Is the formatting you used consistent with the rest of the repo/files?
    • Does the PR add or change something in the UI? If so, add an output snippet/picture reflecting the changes.
    • Is your PR up-to-date with the base branch?

  • Do not mark a PR as ready or assign reviewers until you have done the following:

    • The PR is up-to-date with the base branch,
    • You have completed the self-review,
    • The PR is passing CI, and
    • You have confirmed that any newly added tests are running on CI (by inspecting CI output).

  • Towards the end of your work day, if you have not opened any PRs, look at your local work and ask yourself if any subset of it is “obviously an improvement” or “obviously correct.” If so, consider pulling that chunk of work out to get it reviewed and merged so that everyone can work from that common base as soon as possible. Conducting this self-review will be easier if you carefully maintain your git history.

Reviewing PRs:

  • It is best not to spend much time reviewing PRs failing CI (unless you have been asked to provide early, quick feedback, which is good practice if you are new to a project). Just ask the PR author to get it to the point where it passes CI before requesting a review.

  • If the change is too unwieldy for you to review in one chunk, ask the author to break it into multiple PRs or reorganize the commit history so that each commit is logically reviewable on its own. If you do this, it is important to give specific instructions about how you would like it broken apart. For example, you can say, “pull the refactoring to module X out into its PR and the fix to bug Y into its own PR.” Ideally, PR authors are already doing this during self-review.

  • Things to check for in every PR:

    • If it’s fixing a bug, is there a minimal and focused test demonstrating the bug added? If not, is it reasonable to ask for one?
    • If it’s a new feature, is there a test demonstrating the use of the new feature?
    • Has the documentation been updated appropriately for this PR?
    • Formatting/naming/style convention errors? Ideally, CI should check this.
    • Was dead code introduced?

  • If everything looks good on the PR, but you have some minor nits (variable names, small refactor, etc.), then DO NOT BLOCK MERGING IT. That means do not press the “Request Changes” button. You can either:

    • Press “Approve,” but state very clearly that you expect your changes to be addressed before the merge (check that “auto-merge” is not enabled). Note that you are trusting the PR author to do this.
    • Press “Comment,” saying what you would like changed, and ask for a re-review or allow someone else to approve it.

Addressing Changes:

  • For each requested change, you should take one or more of the following actions depending on the scenario, and always make sure you indicate that you have seen and handled the comment in the GitHub interface:

    • The requested change is unclear: ask for more clarification (if this happens via DM, summarize the conversation on GitHub).
    • The comment is just a question where they don’t understand the code, or they request a change that makes no sense: answer their question directly (DM OK, but make sure to summarize on GitHub) and consider adding a comment to the code so that others do not have the same confusion.
    • The requested change is small or necessary for the PR's correctness: make the requested change, and respond with “Done” or “Fixed” on GitHub to let them know you saw the conversation.
    • The requested change is large, would be scope creep, and is not necessary for the correctness of this PR: make a new issue for the requested change (or find an existing issue), and link to it on the comment.

  • GITHUB GOTCHA: When you are working with GitHub, if you force-push a branch (instead of merging), GitHub does not always know how to update the comments that existed before to display over the new version of the history (even if it’s the same contents). So you effectively lose all the conversation on GitHub up to that point. Better to merge into the base branch and keep the history (only fast-forward) so the GitHub conversations remain in context.

  • Do not allow scope creep:

    • Only make the changes requested.
    • Do not implement new features after requesting a review. You can continue to develop locally on a different branch if needed.
    • If the change requested would be too much scope creep, then make a new issue for it and link to the said issue instead.

  • Make sure CI is passing again.

  • Re-request review from the party that requested changes.

  • Note about trust: if the PR reviewer presses “Approve” but requests changes, and you do not address them, you will lose their trust.

Closing Remarks

This post does not discuss how to actually write code, just how to collaborate with others via GitHub when writing code. At RV, we try not to enforce any particular development style on people on their machines: we encourage teammates to use the IDE they want and manage their git history how they want. We only try to enforce basic sanity on GitHub (squash merges only for linear history, require branches to be up to date, etc.). One true thing is that you should know your tools no matter what technique you use to program. Take time to understand Git, IDE, testing tools, CI scripts, etc., to be an effective developer. Chances are, if you have some confusion or general pain point when doing development, the others on your team have already found a way to solve that problem! If not, then the team should discuss and address it.

A good read about developing code (short, <2 minutes) is “Simple, correct, fast: in that order” by Drew DeVault. In brief, at RV, we focus on producing code that is: “correct; (simple | fast); complete: in that order.” Note that we differ from the above article in that correctness is prioritized (we are producing software for quality assurance, after all), and performance is prioritized to be balanced with simplicity (our software is compute-intensive, so performance is a deliverable). Completeness is added (the idea of “handling all cases”). Following this philosophy when making and reviewing PRs has led to more maintainable code.

This post does not cover the use of CI (continuous integration) in aiding collaborative coding. Perhaps aiding is too weak a word here; fast, smooth, and ubiquitous CI is essential when developing in a collaborative setting. There are few worse things than pulling in someone else’s changes and suddenly having your local checkout stop working because of breakage introduced by them. At Runtime Verification, we have an extensive and thorough CI infrastructure and emphasize automating as much of the development process as possible. Our projects are complex and span dozens of repos, with internal dependency chains up to 6 or 7 repos deep in some cases. As much as possible, we test changes downstream early and push updates downstream automatically so that all repos use the most recent version of upstream dependencies. Perhaps we will write a post describing our CI setup in the future.

In conclusion, there are many ways teams might work together to improve productivity and efficiency when working remotely in a team environment. These strategies may only work sometimes, and they may only work for some teams. We hope that by sharing the approach that the developers at Runtime Verification use for code development, you might begin to implement and improve your own productive remote working environment.