Skip to content

Instantly share code, notes, and snippets.

@jcardenes-via

jcardenes-via/README.md

Last active August 15, 2022 15:35
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save jcardenes-via/368de1f8e67adfabcfcd0409f21bd0e9 to your computer and use it in GitHub Desktop.
Save jcardenes-via/368de1f8e67adfabcfcd0409f21bd0e9 to your computer and use it in GitHub Desktop.
Download ZIP
VIA Pull Request Communication Vocabulary
Raw
README.md

Pull Request vocabulary

Tags for Pull Request (PR) comments:

[o] Optional -> Equivalent solution but it is not necessarily adding more value to the code versus the existing implementation. The PR can be approved without taking any action.

[c] Curiosity -> Side comment, not necessarily relevant for the approval of the PR. The PR can be approved without taking any action.

[r] Required -> The comment has to be addressed / fixed by the PR creator before the PR can be approved.

[f] Future -> Request should be added to the product backlog by the PR creator. Confirmation of backlog task is needed before approval of the PR.

Example usage in PR comments: [c] PR comment

example

Code Review Guidelines

At VIA, we use code reviews to improve code quality and accelerate the learning across our technical team. This is consistent with our values, particularly Learning Never Goes Out of Style. Even in the rare case where code is flawless, the review process provides an opportunity for mentorship and collaboration, and diversifies the understanding of code in the code base.

In this document we are going to cover pull requests (PRs) prerequisites for authors and code review guidelines for reviewers. In particular on code review, we will cover what to look at on the technical side and guidelines for communication. Communication guidelines are consistent with another of our values, Respect a Challenge and Challenge with Respect.

Pull Requests Prerequisites for Authors

  • Prior to starting the development of the PR, make sure to understand the definition of the scope and initial tests required with the relevant Technical Lead (TL).
  • If the pull request is not linked to a JIRA task, please add a small description with a clear scope definition. A one line description should be enough. Otherwise consider splitting the PR into multiple PRs. The code reviewer will appreciate it.
  • Split large PRs into small ones:

    • The ideal size of a PR is less than ~250 lines of code. Split into smaller chunks.1

    • Keep feature branches to small, independent units that can be easily merged to master.

    • Avoid having to create primary branches (e.g., feature/big-change) and secondary branches (e.g., feature/big-change-function1, feature/big-change-function2, etc.), but if you do, please do not let them live more than one week.

    • If PR boundaries are difficult to identify, request support from your TL.

Pull Requests Prerequisites for Reviewers

As a first step, please always review the CodeShip status of the repo, in particular:

  • Confirm that there is an automated test in CodeShip. If not, request the automation to be in place. The author of the PR has the responsibility of doing the update. If time does not allow for the author to do the update, please sync with the relevant TL and repo maintainer on who will do the update and please add a JIRA task.
  • Confirm that the Codeship button is green and review the logs on Codeship to confirm that it is not a false positive. In the event false positives are encountered, please flag it at retrospective.

Technical Review

  • Review and confirm that the code meets the scope defined by the author on the PR. The tests with the PR should help the reviewer identify that the scope is as expected and that it is working.

  • Confirm that the code is tested, the recommended code coverage is ~95%.

  • Confirm that functions are only doing one thing. Otherwise recommend splitting the code into multiple functions.

  • Do not allow functions to have any side effects on the inputs or global variables, as it is difficult to identify when input or global variables are modified.

  • Confirm that functions only have one behavior. Recommend splitting the code into multiple functions if a function has multiple behaviors (e.g., multiple behaviors based on ‘if’ clause).2

  • Identify code repetition and recommend encapsulating the code in a function or adding the code to a utils script/library or updating the structure of the code to avoid repetition (i.e., Don’t Repeat Yourself).

  • Request code splitting if a method has more than 30 lines of code or a class has more than 30 elements. (Note: take these numbers only as a reference. For example, don’t block a PR because a method has 32 lines of code)3

  • In the event of a function with more than 4 positional arguments, recommend using keyword arguments with default values, encapsulating arguments into a data class or refactoring into smaller functions. On one hand, the use of keyword arguments is encouraged. On the other hand, please only use data classes if there are pieces of information that go together, but don't try to hide all of the positional arguments in a data class.4

Communication Review 5 6 7

  • While doing a PR consider starting with the high-level changes (main concepts). If the main scope of the PR wasn't accomplished, wait for the changes to be done and then, in the following round, go for the more specific ones.

  • Provide code samples as suggestions:

    • Don’t provide more than three samples in one round of review.
    • Provide samples for comments that are clear and uncontroversial.

  • Avoid saying ”you” in a comment:

    • It takes the concentration away from the code to the person who wrote it.

    • A spelling mistake can easily be mentioned as:

      successfuly -> successfully

  • Use the communication framework in PRs to clearly identify the expectations around your comments [Optional], [Curiosity], [Required], [Future].

  • Don't aim for perfection, focus on the high impact updates as requirements, and any suggestions label as a Curiosity or Optional suggestion.

  • Don’t comment on every occurrence of a repeated pattern/errors. Comment in one place and request the change throughout.

  • On code style, comment once and provide a link to a formatting library for the machine to do the work. Ideally automation should have already taken care of styling. Python example:

  $ pip install git+git://github.com/psf/black
  $ python -m black {source_file_or_directory}
  • Offer SINCERE praise:
    • Reinforce positive behavior, especially when reviewing code of a newly joined developer
  • If you sense the review moving to a stalemate, handle it proactively. Signs of a stalemate coming:
    • Tone of the conversation becomes tense or hostile
    • The number of notes per round are not going down
    • You are getting push back on many notes
    • Actions:
      • Contact Tech Lead
      • Pair program the next round of edits
      • Consider a design review
      • Request other developers to review the code

Notes

1: Best Practices for Peer Code Review

2: Is it wrong to use a boolean parameter to determine behavior? - Software Engineering Stack Exchange

3: Rule of 30 – When is a Method, Class or Subsystem Too Big? - DZone Java

4: Are there guidelines on how many parameters a function should accept? - Software Engineering Stack Exchange

5: Akash Singh (VIAneer), Code Review Best Practices, February 2019

6: Mychael Lynch, How to do code reviews like a human (part one), 2017

7: Mychael Lynch, How to do code reviews like a human (part two), 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment