Posted by John Morahan on November 21, 2008 at 3:54pm
Jump to:
| Project: | Coder Tough Love |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
Issue Summary
The sentence case rule gave me a false positive on this line:
<?php
$header = array(t('Network'), t('Profile'));
?>Looks like it's treating them as parts of a single string; in fact, they are headers for separate table columns.
Comments
#1
Yeah, this is one of those situations where it's a known false positive, and how to get rid of it is a quandary. In previous versions of Coder, that would come through as one string of "Network Profile". Coder Tough Love would treat it as a sentence, and fail it. In the 2.x version of Coder, we'll get that through as an array of "Network" and "Profile", but CTL will munge that into a single string (primarily as a quickie forward-port from 1.x to 2.x API) and, yep, fail it again.
Going forward, we could check each individual array element, and that would allow the above through (as the code will ignore "sentences" of just one word, as well as the first word from every "sentence") without complaint. I worry that it would then start failing on stuff like "This is a ". $dynamic . " string which, granted, shouldn't ever exist."
#2
Now that the testbot is using this module to do code reviews, I'm seeing large numbers of failures generated by this false positive. My suggestion is that if this can't be fixed, the check should be removed. The hundreds of errors I see in the testbot logs that are generated by this and other false positives obscure any real problems that may exist.
#3
As a general FYI, I'm pretty vehement about *not* removing this check. Coder Tough Love has, as part of its documentation, the admonition that you SHOULD EXPECT false positives, and its mentality is that even one correction in a sea of false positives means it is working properly. I would much rather have it removed FROM USE ON DRUPAL.ORG than removing a check that does find valid corrections.
#4
In general, I agree with your position. And I find this module very useful, which is why it matters to me. However, when I am seeing >100 false positives about "sentence case" without any real positives, that message ceases to be meaningful; it's unlikely I will wade through those results again to catch any problems that may be introduced in the future. If real positives outnumbered false positives on any specific test like "sentence case", I would be fine with the false positives.
I'll try to take a look at the code and see if I can improve the test for sentence case.
Regardless, I realize this isn't the place to discuss how the testbot interprets the results of this module, and I'll take that part of the discussion to the testbot queue.
#5
I agree that some false positives are to be expected, but we should be concerned about losing the signal for the noise. I am also seeing this false positive on the following line:
<?phpstatic $primary_mime_type = array('TEXT', 'MULTIPART', 'MESSAGE', 'APPLICATION', 'AUDIO', 'IMAGE', 'VIDEO', 'OTHER');
?>
#6
How crazy is this... Do you know what the result is of sooo many fails positives? I'm would disable the module as it ONLY complains fails positives and this makes the module useless. I will no longer review d.o coder warnings as i know they are 99% wrong. Therefore I miss the 1% of propper warnings. This is the reesult... Nobody cares about code style warnings any longer.
#7
#6: We are working on removing the false positives on the testbots and moving the "official" rules to coder.