Dreditor could easily flag the non coding standards compliant trailing whitespaces. Patch attached. I hesitated between red and blue, but went with blue for consistency since red is used for the old lines, and the blue thick squares stand out pretty well on my screen. Middots are used in place of spaces so they can be included when pasting a code snippet.

Dreditor with trailing spaces detection

It took no time to find some examples from the RTBC queue ;)
#524728-98: Refactor install.php to allow Drupal to be installed from the command line
#651902-11: Allow ESI tie in
#623320-2: AJAX 'css' command unimplemented in misc/ajax.js

CommentFileSizeAuthor
#24 dreditor-HEAD.tabs_.patch2.49 KBsun
#22 detect_trailing_spaces_22.patch2.43 KBscor
#18 detect_trailing_spaces_18.patch2.42 KBscor
#17 detect_trailing_spaces_17_lite.patch2.39 KBscor
#16 detect_trailing_spaces_16.patch4.68 KBscor
#15 detect_trailing_spaces_15.patch4.57 KBscor
#14 detect_trailing_spaces_14.patch4.56 KBscor
#11 detect_trailing_spaces_11.patch2.08 KBscor
#5 detect_trailing_spaces_5.patch1.69 KBscor
#4 detect_trailing_spaces_3.patch1.69 KBscor
detect_trailing_spaces_screen.png59.88 KBscor
detect_trailing_spaces.patch1.63 KBscor

Comments

scor’s picture

scor commented
Status: Active » Needs review
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Coolio! :)

+++ dreditor.user.js	10 Dec 2009 17:34:43 -0000
@@ -872,6 +872,14 @@ Drupal.dreditor.patchReview.behaviors.se
+      line = line.replace(/^(.*[\S])([\s]+)$/, function (full, match1, match2) {

Why don't we simply test for trailing \s+$ ?

+++ dreditor.user.js	10 Dec 2009 17:34:43 -0000
@@ -872,6 +872,14 @@ Drupal.dreditor.patchReview.behaviors.se
+        var trailer = match2.replace(/(\s)/g, '�');
+        return match1 + '<span class="trailing-whitespace">' + trailer + '</span>';

If the primary expression cannot be cut down (as questioned above), then we still can skip the trailer variable in here and replace directly. Also, the (\s) does not need to be a subpattern, simply \s is sufficient.

I'm on crack. Are you, too?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Having just commented on #601150-145: Improved UI for contextual links...

I wonder whether we could automate this? I'm a bit sick of writing

(and elsewhere) Trailing white-space here.

or

Missing newline at the end of file.

all over again.

What if Dreditor would auto-add comments for those?

Of course, regarding white-space, only the first occurrence.

scor’s picture

scor commented
StatusFileSize
new detect_trailing_spaces_3.patch1.69 KB

Why don't we simply test for trailing \s+$ ?

We need \S to ensure there is a non whitespace right before we start grouping all the ending whitespaces, hence the \S in the end of the subpattern 1. Then we need the second pattern \s+because we latter replace every single \s with a middot in the function.

I rerolled without the [ and ] which were not necessary as you pointed out.

scor’s picture

scor commented
StatusFileSize
new detect_trailing_spaces_5.patch1.69 KB

and now without the trailer variable.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Aside from that question, I also think that the middot will be confusing. Taking your example at #524728-101: Refactor install.php to allow Drupal to be installed from the command line...

If you would have selected multiple, detached code lines for the comment, then the output would have looked like this:

+++ includes/install.core.inc
@@ -0,0 +1,1728 @@
+    include_once DRUPAL_ROOT . '/' . $profile->uri;
+····
...
+    $details = install_profile_info($profile->name);

The purpose and meaning of the current "ellipsis" is mostly clear for everyone, I think. But with those middots, I (a) have troubles to distinguish them and (b) have troubles to understand their meaning.

Thoughts?

scor’s picture

scor commented

I like the idea of adding a whitespace notice as part of the pasted content. Maybe just a line saying "This patch contains some whitespaces" with a link to the dreditor project page? then it's up to people to use the right tools for the right job ;)

scor’s picture

scor commented

re middots, I did a search on wikipedia and that seemed to be the most common way to mark a whitespace in typography. But we're not really doing typography here, we're doing patch review, so maybe something like ◊ would work? is it more understandable?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Testing...

+++ includes/install.core.inc
@@ -0,0 +1,1728 @@
+    include_once DRUPAL_ROOT . '/' . $profile->uri;◊
+◊◊◊◊
...
+    $details = install_profile_info($profile->name);
scor’s picture

scor commented

originally I didn't think of the middots and was happy enough with the thick coloring in the patch review, but then I wanted to review a patch and point it out in the comment: that's when I realized we needed some special character. Which is btw the reason for the function to be there, before that I had simple one liner:

      line = line.replace(/^(.*\S)(\s+)$/, '$1<span class="trailing-whitespace">$2</span>');
scor’s picture

scor commented
Title: Detect trailing spaces during patch review » Detect trailing spaces and no eof newline during patch review
StatusFileSize
new detect_trailing_spaces_11.patch2.08 KB

now with "no newline at end of file" detection.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Detect trailing spaces and no eof newline during patch review » Detect trailing spaces during patch review

Actually, there are chars for this. However, not to be found in typography, but word-processing. ;)

+++ includes/install.core.inc
@@ -0,0 +1,1728 @@
+    include_once DRUPAL_ROOT . '/' . $profile->uri; ¶
+    ¶
...
+    $details = install_profile_info($profile->name);

+++ includes/install.core.inc
@@ -0,0 +1,1728 @@
+    include_once DRUPAL_ROOT . '/' . $profile->uri; «
+    «
...
+    $details = install_profile_info($profile->name);

+++ includes/install.core.inc
@@ -0,0 +1,1728 @@
+    include_once DRUPAL_ROOT . '/' . $profile->uri; ¦
+    ¦
...
+    $details = install_profile_info($profile->name);

+++ includes/install.core.inc
@@ -0,0 +1,1728 @@
+    include_once DRUPAL_ROOT . '/' . $profile->uri; ←
+    ←
...
+    $details = install_profile_info($profile->name);
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Detect trailing spaces during patch review » Auto-detect trailing white-space and missing EOF newline in patch review

oopsie, reverting title.

scor’s picture

scor commented
StatusFileSize
new detect_trailing_spaces_14.patch4.56 KB

@ some of these are more end of line type of chars.

This patch adds a comment to the pasted content when occurrences of whitespaces and/or missing newline at end of file are found. I'm no good at javascript and not familiar with the dreditor code base so let me know if I can improve the patch.

scor’s picture

scor commented
StatusFileSize
new detect_trailing_spaces_15.patch4.57 KB

caught myself submitting a patch with whitespaces... arghhh.

scor’s picture

scor commented
StatusFileSize
new detect_trailing_spaces_16.patch4.68 KB

I agree middots are not so obvious when compared to dots. I like sun's suggestion in #12. However I'm not happy with any of the characters but I cannot come up with a better one. I think the best is probably a character which developers are not used to see in code, like ¶.

The newline detection will give false positives for those lines which are already present in HEAD, but once #606526: [Needs rollback] Remove trailing whitespaces and add newlines at end of files is committed this should not happen. For that reason I don't think it's worth implementing a logic to detect if the no new line of EOF is added or removed. Let's just highlight them regardless.

scor’s picture

scor commented
StatusFileSize
new detect_trailing_spaces_17_lite.patch2.39 KB

I'm leaving out the automatic report for now and we can follow up in a separate issue, otherwise this discussion will drag for ever. Let's get this committed so people can benefit from the improved patch review interface.

scor’s picture

scor commented
StatusFileSize
new detect_trailing_spaces_18.patch2.42 KB

rerolling

scor’s picture

scor commented

@sun: #606526: [Needs rollback] Remove trailing whitespaces and add newlines at end of files got committed - now that we have a clean, whitespace free HEAD, it would be a good time to get this in dreditor :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

It would make much more sense to me to make this feature part of either Drupal.org module (an auto-validation on issue form submit) or Project issue file test module (to flag it as a testbot failure). Not everyone who posts, reviews, and commits patches uses Dreditor.

scor’s picture

scor commented

True, and Coder is supposed to be integrated with PIFR 2.x but I'm not sure it's 100% working. At first I thought writing a patch for dreditor was easier than for the whole PIFR system, and easier/quicker to deploy (pending on sun's commit). But I agree with your point webchick ;) and this is probably getting less valuable now that we'll have coder integration on d.o soon.

scor’s picture

scor commented
StatusFileSize
new detect_trailing_spaces_22.patch2.43 KB

rerolling (and removing unused counter from previous version of the patch).

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Fixed

Thanks for reporting, reviewing, and testing! Committed http://drupal.org/cvs?commit=337232

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Fixed » Needs review
StatusFileSize
new dreditor-HEAD.tabs_.patch2.49 KB

Quick follow-up:

http://drupal.org/node/199957#comment-688319

scor’s picture

scor commented

The pinkish background doesn't strike out as much as the blue one, but I tried this patch and it works!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Fixed

Thanks for reporting, reviewing, and testing! Committed http://drupal.org/cvs?commit=337374

Status: Fixed » Closed (fixed)

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