Development Code Review Procedures and Guidelines

Development Code Review Procedures and Guidelines

This topic should give you as a developer a brief overview about what you should do, must do, aswell as can and can not do. What you as devepoler can expect from Armbian and what we expect from you.

Requirements:

Even though you may already be a developer, just to make sure, here is an outline of the expectations for this process:

Review Approve Merge
Github ID :white_check_mark: :x: :x:
Armbian Github Organisation Collaborator :white_check_mark: :white_check_mark: :x:
Armbian Github Organisation Member :white_check_mark: :white_check_mark: :white_check_mark:

Armbian Organization Members are required to have:

You should know development basics like how to get an Armbian image running on your hardware, do basic debugging, building a kernel and how to use the Armbian build system.

Code Review:

Some helpful guidelines for pull requests and code reviews

It has often been said that programming is part art, part science - that is because lots of times there is no single, simple solution to a problem. Or if there is, we might not know about it. There is also an infamous joke that if there are n developers in the room, then there are n+1 opinions on how things should be done. That being said, here are some guidelines that should prevent friction when submitting or reviewing code.

The most important thing

The code has to work.

Unless you open a PR (“pull request”) as a work in progress, the code should be built and tested on a device or emulator. Do not rely on CI test automation!

If you have touched the build files and changed build setup, it is useful to test the whole build from scratch (clean build) and all of the types and flavours. If you updated external libraries, test the pertaining features. If you changed the build version, make a build and test that the version is correct.

Having people review your code is one thing, but you should not expect them to also test the code for you when not explicitly asked for.

Context

One important thing that lots of guidelines forget to mention is the context of the pull request: Sometimes it is a big refactor, sometimes it is a new feature, sometimes it is a bugfix. Some of those might be more urgent than others, and sometimes you might be under pressure to ship ASAP so the code might not be perfect or there will not be any tests or code might not be extendable. That is ok.

Everyone

  • There is no perfect code: good enough is usually good enough. That being said, try to keep the number of WTFs per minute to a minimum.
  • Accept that many programming decisions are opinions. Discuss trade-offs, which you prefer, and reach a resolution quickly.
  • Ask for clarification. ("I didn't understand. Can you clarify?")
  • Offer clarification, explain the decisions you made to reach a solution in question. Avoid using terms that could be seen as referring to personal traits. ("dumb", "stupid"). Assume everyone is intelligent and well-meaning.
  • Be humble. ("I'm not sure - let's look it up.")
  • Do not use hyperbole ("always", "never", "endlessly", "nothing"). Avoid sarcasm.
  • Remember that you are both on the same side - the goal is to make the code better. Understand that sometimes your ideas will be overruled. Even if you do turn out to be right, do not take revenge or say, "I told you so".
  • Talk synchronously (e.g. chat, screensharing, in person) if there are too many "I didn't understand" or "Alternative solution:" comments. Pull requests should not be the place for long discussions, architectural or otherwise.
  • Put notes on what is missing or could be improved in the PR description or comments. You can also make a Jira ticket with discussions points and possible problems or things to do and discuss it offline.

    • As a Code Submitter

  • PRs should be about one thing. If you do multiple things in one PR, it is hard to review. If you are fixing stuff as you go, you might want to make atomic commits and then cherry-pick those commits into separate branches, leaving the PR clean.
  • Try to keep the PRs small.
  • Having a PR description is useful. Additionally, you can also link to the Jira ticket.
  • Ideally, the PR should be finished when submitted. If the PR is work in progress, add (WIP) or [WIP] to the title.
  • You should have tests that at least cover the logic, and ideally also cover the input/output parameters and methods. (depends on context)
  • Make sure to add a documentation PR when needed https://github.com/armbian/documentation

    • As a Reviewer

  • Reviewing code is part of a normal workday. You should check for open/updated PRs / Jira ticket as often as you can.
  • Ask, do not tell. (“What do you think about trying…?” rather than “Don’t do…”)
  • Offer ways to simplify or improve code.
  • Code beautification and refactoring ought to be tasks in the next sprint, except for obvious and easy-to-fix things.
  • Communicate which ideas you feel strongly about and those you do not. Explain your reasons why code should be changed. (Not in line with the style guide? A personal preference?)
  • If you disagree strongly, consider giving it a few minutes before responding; think before you react.
  • Offer alternative implementations, but assume the author already considered them. ("What do you think about using a custom validator here?")
  • If discussions turn too theoretical or touch big architectural questions, move the discussion offline. In the meantime, let the author make the final decision on alternative implementations.
  • Do not keep the code hostage. Keep in mind the context and the most important thing - does it work?

    • Merging a merge request

    Before making the decision to merge:

  • Set a milestone.
  • Consider warnings and errors from Github CI bots, code quality bots, and other reports. Unless a strong case can be made for the violation, these should be resolved before merging. A comment must be posted if the PR is merged with any failed job.
  • If the PR contains both quality and non-quality-related changes, the PR should be merged by the relevant maintainer or senior software engineer after the quality related changes are approved by more then one software engineer.

    • If a pull request is fundamentally ready, but needs only trivial fixes (such as typos), consider demonstrating a bias for action by making those changes directly without going back to the author. You can do this by using the suggest changes feature to apply your own suggestions to the pull request.

    Note that:

  • merging is limited to Armbian Github Organization members, you can apply here to become one, if you have not already been invited.
  • If the changes are not straightforward, please prefer allowing the author to make the change.
  • Before applying suggestions, edit the pull request to make sure squash and merge is enabled

    • Authors are not authorized to merge their own pull requests and need to seek approval from another maintainer / developer to merge.

    Armbian's Assistance

    If you have questions about being a developer or want to learn more and deeper insights about the build framework, Armbian will try to guide you to the appropriate documentation or information in a best-effort fashion. If time allows, at our descrection, we will try our best to explain and teach you personally various aspects about our processes. If best effort guidance is not enough, contact us for professional assistance.

    If you have any concerns please do not hesitate to reach out via forums, IRC or Discord. Armbian cares about the people who care about Armbian  :)

    References / Sources: