Closed (fixed)
Project:
Coder Tough Love
Version:
6.x-1.x-dev
Component:
Miscellaneous
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
3 Dec 2011 at 19:42 UTC
Updated:
19 Dec 2011 at 12:10 UTC
I've recently spent a lot of time nitpicking doxygen for core patches, and this module already covers a lot of deficiencies in Coder Review. Plus, now that it's running for PIFR, it's going to get a lot of visibility. A longterm strategy might be to merge the rules this module provides under the main Coder umbrella, but in the short- and middle-term, I'd love to help maintain the module and fix issues that come up related to the Coder Advisory Reviews.
Comments
Comment #1
xjmOh, here's a résumé:
includes/.Comment #2
morbus iffHrm. I'm surprised that it's running under PIFR now actually - I thought the end-result was that it *wasn't* going to.
With that said, I think I told the person whose name I can't remember that I have no interest in maintaining versions for "old" core versions (i.e., 6.x), so if you're planning on doing that, be my guest. Added. Generally speaking, I'm not really... "enthusiastic" anymore about CTL, mainly because it can only ever catch "simple" things, and there are far more things Wrong, design-wise, with contrib, that can't easily be caught by simple regexes. Remember, its original goal was to automate all the dumb/simple things that chx and my Drupal Tough Love reviews were commenting on, leaving us to really lament about the Big Problems.
Note that the last time I ran this code, it required a beta version of Coder so I'm not even 100% sure if it properly interfaces with the Coder interface anymore!
Finally, http://drupal.org/node/947496. Your funeral, bub.
Comment #3
morbus iffIncidentally, have a recent issue where I can see a CTL report for a patch?
Comment #4
xjmWell, you can see the results on a branch test here:
http://qa.drupal.org/pifr/test/158434
(Click the "Code review" tab; it doesn't seem to have its own path. See also http://jthorson.doesdrupal.com/node/34.)
So I noticed right away that there's two issues to fix based on that branch test: #737372: Overly aggressive matching on "date" pattern, and supporting the new standard in #711918: Documentation standard for @param and @return data types (which I don't see an issue for in the project yet). The messages about indentation are a false positive from having datatypes specified for return values.
What about your interest in a D7 branch? I see someone has submitted a D7 patch, but I wasn't sure what its status was.
Comment #5
morbus iffRuling on #711918 should be part of coder.module, as it's an official standard. CTL is primarily for "unofficial" standards - things that happen in core already but which don't have any existing documentation. With that said, #711918 would require a CTL *removal*, namely the one about "@param and @return syntax does not indicate the data type." But, really, it's all a clusterfuck, because #711918 only applies to "new code" (being D7, D8) and not "old code", so logistically, #711918 should have no place in D6-based code (such as the CTL that runs under PIFT) because it's a D7 standard! I'm slightly cautious that running Coder in PIFT is a good idea, because of these discrepancies, unless PIFT can intelligently tell Coder/CTL (and they'd then listen) that you're currently checking a D6, D7, or HEAD codebase. (As an example of said clusterfuck: "I spoke to webchick today at PNWDS, and she made it clear to me that in Drupal 7 patches, she does not want to see these data types creeping into the docs."")
As for the D7 branch, see my previous comments about "enthusiastic". I'm not that enthusiastic about CTL anymore - it was only meant to let chx and I focus in on the header design and pattern problems in Contrib and not all the common oopsies that aren't covered by existing documentation. You're welcome to make a D7 release too.
Comment #7
xjmAye, that makes sense. I was thinking it would be best to just back out the "@param and @return syntax does not indicate the data type" rule, plus fix the "indentation" false positive so it properly recognizes that the data type is not the description. Then, perhaps Coder Review can add a rule for it.
Comment #8
klausi@xjm: drupal code sniffer might also be interesting for you: http://drupal.org/project/drupalcs