Pre-integration peer review

Owned by Steve Mattingly (Unlicensed)
Last updated: Aug 18, 2023 by Sayaka Ono
8 min read

Pre-integration review of new and modified technical work products by peer developers is meant to

  1. ensure those work products satisfy team priorities and expectations, and

  2. transform an individual's work products into team products.

Accordingly, you should approve the pull request (PR) if

  1. you judge that the products address team priorities and expectations to the greatest degree reasonable, and

  2. your understanding of, and confidence in, the products are high enough that you agree to be responsible for future work on them.

Otherwise, you should reject the PR with requests for specific changes that would lead to your approval.

In the preceding descriptions, "team priorities" refers to these critical success factors for continued growth and success of the LiteFarm application:

  • Designs that separate concerns

  • Readability, simplicity, and clarity of code and other technical products

  • Appropriate levels of documentation

"Expectations" refers to a more detailed set of guidance, defined later in this document, that are meant to support the three priorities.

"Greatest degree reasonable" indicates that the expectations are highly important and generally relevant, but you should apply your judgment. In particular, be aware that most of the LiteFarm codebase was created before our current priorities and expectations were explicitly communicated. As a result, scenarios similar to the following will be common.

  • A Jira ticket calls for a simple modification to some existing code, which can be accomplished easily.

  • However, the existing code needs refactoring to address current priorities and expectations.

  • Reasonable people might make different tradeoffs concerning how much effort to invest in that refactoring now, versus in the future.

There can be no algorithm or universal policy for handling all such scenarios. But we can expect good results when pre-integration review is a "middle layer" in the following scheme.

  • When you are assigned a ticket where discussion of "how much refactoring?" is appropriate, do not wait until pre-integration review. Raise the question in the team's daily scrum so that we can seek consensus from the entire team.

  • Successful completion of pre-integration review means that multiple teammates have agreed that any relevant team consensus has been satisfied.

  • When there is consensus to delay refactoring to the future, the work should be captured in new Jira ticket(s) that document this technical debt.

Team expectations for technical work products

These largely assume that the work product is source code. As appropriate, adapt and apply them to other technical work products: test strategies, design documentation, database schemas, and so on.

Automated processes use prettier and eslint to enforce basic standards for formatting and syntax conventions, so these "mechanistic" topics will not be addressed here.

Inspired by eslint's rule names, each specific guideline below has a standard kebab-case name. Use these for precise, succinct communication when requesting changes to a PR.

General, for all code

  • Do include a current copyright and license notice at the top of every LiteFarm source file, using the format shown below.

  • Don't include the filename in the notice.

Notes

  1. At the time of this writing, not a single file in LiteFarm fully meets this expectation. While writing the guidance, we learned that using year ranges like 2019-2022 is not ideal.

  2. Each subsequent year that a LiteFarm version is released will be added to the year list in future. When all files are following best practice, this could be automated.

  3. Currently, many LF files have no notice at all.

  4. Currently, many LF files incorrectly state that the software is copyrighted by the Free Software Foundation. (It is the license text that is copyrighted by FSF.)

  5. Currently, many LF files include a filename in the second line of text. Often the filename is incorrect because of renaming files or copy/pasting the notice. Do not include a filename.

Format:

/* * Copyright 2023 LiteFarm.org * This file is part of LiteFarm. * * LiteFarm is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by * the Free Software Foundation, either version 3 of the License, or * (at your option) any later version. * * LiteFarm is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details, see <https://www.gnu.org/licenses/>. */

 

  • Do use comments in JSDoc format to the greatest extent reasonable.

Notes

  1. See https://lite-farm.atlassian.net/wiki/spaces/LITEFARM/pages/1190395905 for examples that include the tags @async, @desc, @enum, @link, @member, @name, @param, @returns, @see, @static.

  2. There is a preferred style for writing the initial description (the first part of the comment, with an implied @desc tag.) Try to emulate the examples; sticklers can consult the grammar-oriented rules here.

  3. LF does not actually run the jsdoc to to generate formatted documentation, and possibly never will. This is still a good format for internal documentation.

  • Do use comments to clarify intent, where JSDoc comments are not appropriate (inside functions for example).

  • Don't make others figure out what you are doing or why.

"Good comments don't repeat the code or explain it. They clarify its intent. Comments should explain, at a higher level of abstraction than the code, what you're trying to do." Steve McConnell, Code Complete

Do resolve warning and error messages reported by development and operations tools to the greatest extent reasonable.

Notes

  1. There are currently many warning or error messages from compilers, browser consoles, etc.

  2. While it may be “safe” to ignore most of them, that’s a very slippery slope. In effect you end up ignoring all of them, including any new ones that might be buried in the pile.

  3. Ideally, messages are resolved by improving the code. In some situations it may be possible and appropriate to “disable” the message. This should be done with caution. The disabling should be for the smallest scope possible. A comment should indicate why the check was disabled rather than changing the code.

  4. When there is consensus to delay this work to the future, the work should be captured in new Jira ticket(s) that document this technical debt.

  • Don't write code using the moment.js library.

  • Do use alternatives.

  • Do refactor code using moment.js to use alternatives, especially JavaScript features that need no custom library.

Notes

  1. LF uses the moment.js library widely.

  2. The maintainers of moment.js have explained reasons behind this, and alternatives available.

Frontend code

Backend code