Review github reply template proposal

During our week in Brno, we discussed what are the things to check during review. I am presenting a proposal for further discussion. The idea is to put this to wiki as a reference so other reviews can make it a comment (reply) template:

  • Ticket number, title and description correct?
  • Does it fix the problem described in the issue?
  • No unrelated changes?
  • Redmine opened and in correct project?
  • No use of .to_sym, .send or other reflection on untrusted inputs.
  • Not missing string extractions (use :mark_translated: true).
  • All string extractions follows our rules.
  • Added appropriate permissions for non-admin users?
  • New code hasn’t been copy-pasted from elsewhere.
  • Do new fields have appropriate validators?
  • No exceptions are swaloved/squashed.
  • Are raised exceptions type of Foreman::Exception or WrappedException?
  • Unit/functional/integration test(s) present?
  • New view templates have .html.erb extensions, not just .erb
  • Mixins use ActiveSupport::Concern instead of class_eval etc.
  • Concerns and new classes are added to app/ and not lib/
  • Apipie documentation is correct: HTTP methods, names of parameters, required true/false
  • Scoped_search definitions are added for new model attributes where appropriate.
  • Scoped_search definitions for virtual fields (:ext_method) use :only_explicit
  • Makes use of logging appropriately?
  • Pass block to logger.debug to lazily evaluate expensive debug statements
  • Deprecations use Foreman::Deprecation with a deadline of latest stable + three versions.
  • Not changing structure or behavior of the public HTTP API?
  • No memory or performance concerns?
  • Any packaging needed?
  • User documentation needed?
  • Demo worthy?
  • Any other actions need to be taken after merge?

Please discuss (add your own ones) and I am going to put this to our page Reviewing patches - Foreman and a link to this page to our PR template. Thanks all!

It looks like saved replies are personal and I can’t find a way to set these per organization/repository.

In nearly all cases reading through the list, these checks look like they could be automated and extract the human element from them. @ekohl has played around with a new app for setting github statuses. What if we operationalized these checks to handle the scale of PRs and eliminate the human element? The rest can be included as a automated comment either whole hog or conditionally (e.g. if a PR includes .to_sym then “No use of .to_sym, .send or other reflection on untrusted inputs.” is included).

As much as I appreciate the automation, I would like this discussion to continue about the list itself. Please think about:

  • What should be removed from the list
  • What is missing in the list above

Good quality checklist is the first step in achieving what we all want :slight_smile:

1 Like

The longer the list is the less likely reviewers will use it when doing reviews, so I would prefer a short(er) list with the most important and common points. I would remove things we already have rubocop or prbot enforcing, and see if there are any others we can start enforcing as well. There are a couple that i don’t recall ever seeing in prs - for example, “New view templates have .html.erb extensions, not just .erb” or “Concerns and new classes are added to app/ and not lib/”. Some of the items could be combined, e.g. "All strings are extracted properly ", scoped search, logging etc. And one final comment, we only do deprecation for 2 releases at most now.

I’d add:

  • needs to be added to API?
  • needs hammer support?
  • needs manual update?
  • needs to be in release notes?

many of them are not something we should request from new/random contributors, but at least the reviewer should open redmine tickets or make sure it will be added somehow

Can you guys move this to PR? It’s not sure where you want it to be - in the template or the document? Thanks.

moved mine, thanks