Coder metrics
Grugnog2 - January 29, 2008 - 07:30
| Project: | Coder |
| Version: | 5.x-2.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (code needs work) |
Description
Here is a patch (well, a module and a small patch to fix the statistics collection) that creates a table of results for the modules selected as the defaults.
| Attachment | Size |
|---|---|
| coder_metrics.patch | 7.73 KB |
#1
- $results = array('#stats' => array('minor' => 0, 'normal' => 0, 'critical' => 0));+ $results = array();
Why is this change needed? The initialization here was done so that the addition of 1 doesn't cause an E_ALL error.
#2
The change definitely made stats work somewhat correctly, but the neither was really the correct solution. Also, I don't think the initialization here could prevent any warnings, since AFAICS we never add 1 to any of these values - 1 is however added to the $results array in do_coder_review(), and this patch adds correct initialization for that array too.
The reason why this was breaking things was because the $result array was being added into the $results array using a simple $results += $result;. This works fine when it is simply adding error items, each with a different key, but it was breaking with the '#stats' item because that key was already in the array and so the new results were not copied over (and definitely not summed, as I suspect was the intention). http://www.php.net/manual/en/language.operators.array.php explains this better.
The patch above improved things, because the initial items at least made it to the $results array - however, it only captured the first nonzero result for each severity, which is obviously not correct.
The attached patch fixes both of these, by actually summing each of the stat values.
We could perhaps simplify this looking a bit by restructuring all these $results arrays so that there is a '#data' item (for the error lines) as well as a '#stats' item. The '#data' item could then be simply added to the $results array using the original technique, although the '#stats' item would still need to be dealt with as in the attached patch. I think refactoring this (if we decide to do it) is a separate patch however, since it touches quite a lot of code, and the current code at least fixes the current bug.
#3
I like where this is going. I know that we briefly discussed this, but I'm thinking that this might be more useful if it's directly integrated into coder, and if these results were the summary results shown on every coder run. They could be put into a collapsible fieldset with links to the individual module results.
Is there an advantage to having it standalone? If the reason to have it as a standalone module is so that people who don't want it, don't get it, I'd like to see a hook added to coder, and let coder_metrics run off the hook. I think that the standalone interface actually doesn't work very well, because it's always show the results for the 'default' settings and I think that many (maybe even most) people use the selection form.
If we hooked the do_coder_reviews loop (before, during, and after) and let modules add stuff to the results (that would then be cached) and let modules extend the output, would this work?
#4
Coder Metrics themselves aside, the result count issue is a bug that can be handled separately. Rather than loop over all the results, however, I simply looped over any #stats elements (minor, normal, critical) and added those values to the parent.
Posted here: http://drupal.org/node/219407
#5
@ Bill in #4, the patch you wrote to correct the $results += $result has been committed to 5.x-dev, and has been ported to 6.x-dev. Any further work on this current issue should take that into consideration.