The Pull Request Checklist

I have this idea of making a checklist for our developers so we can keep our engineering standards high. Currently the checklist is pretty slim and I'm looking for more ideas which can be added.

• Every third-party call is:
  • Done by a circuit breaker
  • Limited by time
  • Handles properly errors
• Every promise / callback / async handles properly errors
• Every edge case is covered by test(s)
• No assumptions about data integrity / consistency were taken
• Potential performance pitfalls were considered
• Commit messages follow the convention

Of course our development methodology is much broader but in this list I want to keep only the most important things.

Thanks :)

Comments

[Sophie The Lionhart]

I think you should check out Codacy. We use it at our current company and I almost love it. I love the concept of it for sure.

It implements code linters and static analyzers on every PR you have so it looks for a lot of the things you have above. Certain things obviously you can't catch with it and need a human touch still, such as the test edge cases though.

Its nice because it allows the person doing the PR to no have to look through all the BS parts of a PR. They only have to focus on things that a linter or analyzer can't catch. Also when it complains that there is a mistake in the PR, it links to education about the mistake so that the developer grows. Its basically free mentoring on the easy stuff, which is the part I love!

I feel there has to be some kind of PR checklist integration for github but I haven't found one. That said I've put in very little effort looking.

The only problem with Codacy is that sometimes it takes a while to get a PR analyzed, like a few hours. Its gotten a lot better recently, but that's one of the pain points.

[Ido Shamun]

Thanks! I'll look into it.

[Sophie The Lionhart]

Yeah, let me know if you end up using it!

[Mikhail Korolev]

Have you considered running any kind of CI/CD service? That's basically almost everything you mentioned. There are different approaches for almost every language out there.

Enforce different rules through code linters, practice checkers and tests. Enforce a rule of never decreasing code coverage. Only merge PR after everything passes.

Ideally, your development process should look something like this (I have no idea what is your stack, let's take a small open-source Rails project as an example):

1. Make local changes you need
2. Before you commit anything, run tests. rake test
   • rspec: generic unit tests, ensuring everything still works
   • Rubocop: complex code linter. Do you want your code to follow a certain style? Describe how you want it to look like. Limit line and method lengths, check for global variables, force to wrap reusable code into separate code blocks.
   • Anything else you want, there are a plenty of gems and different solutions.
   • Make sure the coverage at least remains the same.
3. Commit if everything passes
4. Push to remote, make a PR
5. During the PR process, a CI service, let's say Travis picks your commit up, runs all the tests and marks your commit with a nice green checkmark.
6. Merge into master without worrying. Plus, this can be automated if needed.

Running this cycle even with smaller projects can greatly benefit your code style. If you do it on a regular basis, the "oh my god, another check failed again" will turn into writing code that is initially clean and easy to maintain.

[Ido Shamun]

We are indeed working with CI/CD and following the same best practices as you mentioned. The checklist is meant for the reviewer as a guideline for his/her review. Even with massive automation tools code review is still needed

[Tobias SN]

Use a specific code style, and make everyone follow it. This could be implemented by a formatter to lift the burden off the devs.