Luckily I was able to work with QMS and convince them this new way has benefits miles ahead of where we were, and we updated our policies accordingly. In my situation the only need for per-review checklist would be strictly for policy reasons at this point.
My personal approach is to force upon my team at least one old-style review as new developers join, as appropriate.
#Www qms tor com code#
I personally think the experience of the old style code review provided some benefits over per-commit style - largely related to a fun team-building experience as well as humanized exposure to our checklist. This is not a problem per se (specifically not at all even in the domain of Phabricator) though more an observation with this change. Every developer is trained on where our official document is and are required to review it however having an actual checklist provides a more tangible learning experience - and can take the bite out of "another policy document" by working with the team. Right now this happens through more-senior developers reviewing code and the communication of our guidelines becomes implicit. Looking past documentation policies one of the functions of our code review which we have yet to distinctly recover is that the old-style reviews both communicated and exposed new developers to our guidelines. We fortunately have updated our policies to defer all code reviews being in our Phabricator instance which is overwhelmingly seen as A Good Thing. one of these reviews a month for which the signed checklist is archived into QMS for audits. These checklist items were our coding guidelines which are a mix of lint-able rules as well as higher-level ones (such as "Do comments explain Why instead of What?"). Do you already have this checklist in some form available to all engineers as onboarding documentation (e.g., "guidelines for effective code review", "how to handle errors in ", "choosing between datastructure X and Y", etc)?Īt my company previous code reviews consisted of filling out several checksheets after a group-meeting style review.
What sort of mindset do you imagine reviewers are in if they would ignore an unclear comment without a checklist, but flag it with a checklist?īroadly, this seems like a training/leadership issue to me, not a technical/tooling issue. Surely no reviewer who is properly trained and taking their role seriously can legitimately need a reminder about these things? If a reviewer reads a confusing, unclear comment and doesn't object to it, I can't imagine that's because they didn't have a checklist of 15 things they should be looking for that includes "comments should be useful" and "code should do what it is supposed to do". What advantage are you expecting to gain by building these into Differential as a checklist?įor example, two of the things you list are "Are comments clear and descriptive?" and (approximately) "Does the change do what it's supposed to do?". I would expect them to be, e.g., on a wiki page like "Guidelines for Effective Code Review at ".
0 kommentar(er)
