Review code snippets

The attached patch allows the user to review patches created with the -u option (i.e. unified output). The user just needs to copy and paste the patch into the text box provided at coder/patches. It will only review new or modified lines, i.e. patch lines starting with a "+". We don't want to review removed lines (starting with "-") as we may be providing warnings on lines that the patch is fixing! However, we possibly should be reviewing unchanged lines (ones that start with a space) - it may eliminate part of issue 3 below.

There are a few of wrinkles left to be ironed out:

1. For every patch containing a comment, you will get the "Include the CVS keyword $Id$ in each file." warning. We need a mechanism for disabling this check for patches as they're not complete files. I was thinking that a new attribute on the rule to exclude it from patch reviews may work, but it's only one rule...
2. If a patch section starts part way through a multi-line C-style comment (i.e. starts on a * ... comment line), then the comment is treated as php code and warnings about incorrect indentation may occur. An example can be seen if you review this patch itself! The code at the minute is forced to assume the input is php code (as we don't have opening <?php lines), but there could be a way to override this if the line starts with " * ...".
3. Rules which should only be applied in certain functions won't get invoked most of the time. We could take the function name from the "@@..." line if the patch was created with the -p option. However, the patch only reviews the new/modified lines, i.e. only the lines beginning with a +, so it's possible the function name exists in the patch text as a non-modified line, but we don't catch that situation either. The only way to resolve this (I think) would be to separate out the patch handling reviews completely from the normal file handling reviews.

Feedback or patches on how to rectify the above issues welcome!

The user will also need to be aware that if the pasted lines wrap because the browser window / textarea is too small, then a warning about "trailing spaces" may be given. One way to resolve this issue would be to take the patch from a supplied URL instead, but that would probably envolve using cURL and the need for the PHP cURL extension, which not all users may have installed.

Anyway, the above patch, while won't catch all coding errors, will at least catch most of them I think. Feedback and suggestions welcome!

Cheers,
Stella

Ok here is a newer version of the patch. Issues 2 and 3 should now be fixed.

It now reviews unchanged lines in addition to the modified lines, this combined with additional parsing for the function name for patches created with the -p option, solves the issue of function-dependent checks not being triggered.

The remaining (known) issues are:

1. For every patch containing a comment, you will get the "Include the CVS keyword $Id$ in each file." warning. We need a mechanism for disabling this check for patches as they're not complete files. I was thinking that a new attribute on the rule to exclude it from patch reviews may work, but it's only one rule that is affected (that I know of) and so I would consider this a minor issue.
2. If any of the lines of the pasted code _wrap_ due to the browser window / textarea being too small, then some false warnings will be given. The false warning should be either in relation to "trailing spaces" or the incorrect application of php code rules to quoted strings. I don't know any way of avoiding this, other than taking a patch from a supplied URL. This would probably suit the majority of users, but may require a dependency on the PHP cURL extension. Will look into this further.

Please test and review this patch - all feedback, patches and suggestions welcome!

Cheers,
Stella

Ok, issue 2 is fixed! It's possible to disable wrapping on textareas, so updated patch attached. Issue 1 is still outstanding, but is minor enough, that I'll wait for more feedback and reviews before looking at. We just need a way to disable that one check for patches.

Cheers,
Stella

Ok I thought about the "providing a link to a patch" functionality a bit more and thought it would be a good idea to have it in addition to the textarea method. Since file_get_contents() can get web pages, it uses that instead of any cURL, etc, alternatives.

Issue 1, with the $Id alert triggering when it shouldn't, still remains however. Doug, how would you propose disabling this check?

Cheers,
Stella

whoa. Stunning.

Note: AFAIK, cURL is more reliable than file_get_contents(), which requires PHP's fopen_wrappers to be enabled.

I haven't read your link yet, but if that is the case then both solutions (cURL and file_get_contents()) require something to be enabled or installed. Perhaps a call to function_exists() on file_get_contents() and/or curl_init() could be used to determine which method to use.

Cheers,
Stella

this patch seems like a great idea, but it doesn't apply cleanly at all to the current HEAD of coder. Looks like 6 hunks failed. I'll see if I can reroll, but I'm not at all familiar with the module or patch code :)

Just because you cannot re-roll this patch, this does not mean that this won't fix.

Patch re-rolled. See attached.

In addition to issue 1 (with the $Id alert triggering when it shouldn't), there's now also a "missing @file block" warning triggered - this is a new rule that was added since the original patch. I think we just need a way to specify if certain checks should be applied to patches or not. If we decide to do it this way, then I think a separate patch should be required for that.

Cheers,
Stella

Stella, this is great! I've added the #filename and #filename-not rule directives, so that we can include or exclude a rule based on the filename, and I added the #filename-not option to the CVS Id check.

One small change that will allow people to test patches with coder using the drush command line, for example:

$ drush coder http://drupal.org/files/issues/185373.patch

$ drush coder /src/my.patch

This somewhat works without the Drush patch #297611: Command line path arguments need the current directory, but works better with that patch.

Excellent! I've tested your changes (both the file directive and drush changes) and they work perfectly. I've attached another version of the patch with one small change to includes/coder_comment.inc I added in a #filename-not setting on patch files to the "_coder_comment_missing_file_block" rule (all files need to have a @file block) as this rule should not be run for patch files.

I'm happy with this patch now. Shall I create a Drupal 5 version? Or are all new features now just going into D6 / D7 branches?

Cheers,
Stella

Looks great, please commit it. As for a 5.x version, it's not required.

Committed! Thanks Doug.

@sun in #9: I reviewed Karl's troubles with getting this thing rerolled, and I actually told him to mark it as 'wont fix' and stand back to see how fast it got attended to, rerolled, and committed.

I'm happy to see this one in, and it's going to be of great value to the community in general. Marking something as won't fix is not a normal step in the chain of getting patches committed, but in this case I'm glad to see my plan worked. It's typically 80% faster for the original patch's creator to re-roll than for someone who wasn't in that headspace at the time.

Cool beans for all. This is an awesome improvement to the coder.module, of which I already love and adore.

Cheers!

I actually told him to mark it as 'wont fix' and stand back

please do not abuse the system :)

I was already working on a re-roll when the issue was marked as "won't fix", so tbh, I don't think marking it as such helped the patch to be provided any faster. Please don't abuse the system like that again. You'll just find that you'll irritate a lot of people or, even worse, that issue statuses become meaningless.

Stella

Automatically closed -- issue fixed for two weeks with no activity.