Review github reply template proposal


#1

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!


#2

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

https://help.github.com/en/articles/creating-a-saved-reply


#3

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).


#4

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:


#5

#6

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.


#7

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


#8

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


#9

moved mine, thanks