Posted by heddn on May 31, 2014 at 8:25pm
Taking a page from https://drupal.org/issue-summaries, I've added a section to Increasing efficiency in manual code reviews that provides a template for reviewers. It is sort of a checklist for reviewers. This should help novice reviewers that working on the bonus tag and experienced reviewers with their efficiency. Feedback appreciated.
Part of my hope is that it will give reviewers more direction on what areas they should cover and in general help make reviews easier.
Comments
I like this. I started using
I like this. I started using the checklist tab on pareview.sh, but that is awkward. I may tweak this a little to add in (+) which I have been using for non-blockers, but important issues to fix.
@mpdonadio, please do add and
@mpdonadio, please do add and make it better.
Looks good to me! Usually my
Looks good to me! Usually my comments tend to get long, so I don't want to make them even longer with this template. And by the time I comment on an issue I hope that the obvious stuff has been addressed anyway already.
Thinking about it - most upper points in the template could be used in the issue summary of an application, as many points can be assessed objectively. That way the basic checks are not hidden somewhere in the comments and reviewers see immediately if somebody already verified the stuff. The usual nitty gritty detail code review comments could still stay in the comments.
On the other hand we may want that each reviewer checks the points independently, so i'm not sure ...
I also haven't been using
I also haven't been using this, as I have been working on my own template in combination with some small changes to the CLI version of pareview.sh (@klausi, I'll send a pull request if I get it to a point where others may find it useful).
I don't think that adding this to the issue summary is a good idea. Things change. People may add third-party libraries, and fixes may introduce new security problems.
I think making this template a new discussion page in the group would be good to serve a few purposes. The biggest is that we could link to it from https://drupal.org/node/1975228, and request that people use the template to get a review bonus. This would help cut down on the number of people just posting the automated review results and those just doing a cursory glance at the code. Yes, all of this is requested in the pages on how to review, but having someone actually fill out a checklist that is easy to find should help get better reviews.
I tool the liberty to split
I tool the liberty to split this out to https://groups.drupal.org/node/427683 to make it easier to edit, especially to reproduce the code section.
PAReview: Review Template
I think it is great to have a template for reviews.
However, I think the current template can be improved.
I have three suggestions:
It is not always clear which of the "Yes/No" indicators that indicates a positive review.
For instance, the point: "Code too short/too simple" in the present template is expanded into "Follows the guidelines for project length and complexity." So does a "Yes" mean that the code is too short, or does it mean that the code follows the guidelines? I suggest that each bullet point is written so that a positive review of that particular point always results in a "Yes".
I think the markup with a two level bullet list inside a description list is awkward. Why not use a description list for the template?
I think the list should start with of an assesment of the applicant ("Individual user account" and the applicant having "No multiple applications"), followed by a general assessment of the applications viability ("No duplication") and some formal points regarding the status of the repo configuration and its contents ("Master Branch", "Licensing" and "3rd party code"). Then at the end, I will have the points that actually involves looking at the contents of what is submitted ("README.txt", "Code long/complex enough for review", "Secure code", and "Coding style & Drupal API usage").
Below the line is how want to implemented these suggestions:
Automated Review
Best practice issues identified by pareview.sh / drupalcs / coder. Please don't copy/paste all of the results unless they are short. If there are a lot, then post a link to the automated review and mention that problems should be addressed.
Manual Review
The starred items (*) are fairly big issues and warrant going back to Needs Review. The rest of the comments in the code walkthrough are recommendations.
Please don't remove the security tag, we keep that for statistics and to show examples of security problems.
The guidelines for project length url don't use the good domain
it goes to : https://drupal.org/node/195848 since it should be https://groups.drupal.org/node/195848
I saw that on this issue https://www.drupal.org/node/2113649#27 and i assume it should happen on others places.
I don't have the access level
I don't have the access level required to correct here, but I hope people use the actual template (and not this draft) as a starting point. The link is correct in the actual template.
In the review you're linking to, I used my personal draft (which also contained the error). I don't think I used that for more than this single review.
I like it
I noticed the same problems when trying to use it the other day. I copied in this markup.
Drop "No multiple applications" from template
I suggest dropping No multiple applications from the template, since the PA Robot does this check automatically.(Done.)I also considered dropping Master Branch. The PA Robot checks that a major version branch (and not "master") is used. However, until this feature request is implemented, a manual check is also needed.
Link directly from the group page
Can we get a link to the review template placed on the g.d.o front page?
https://groups.drupal.org/node/427683
In the "Tools" section
In the "Tools" section there's already a link to a template that's a different template.
Let's figure out where to link to and then update that block.
knaddison blog | Morris Animal Foundation