CVS HEAD come with some not proper line break or empty line, which can cleanup with scripts/code-clean.sh.

Patch via CVS HEAD, directly apply scripts/code-clean.sh without any other change.

Files: 
CommentFileSizeAuthor
#30 drupal-HEAD-1284611436.patch32.28 KBhswong3i
PASSED: [[SimpleTest]]: [MySQL] 24,791 pass(es).
[ View ]
#27 drupal-HEAD-1284428012.patch232.98 KBhswong3i
PASSED: [[SimpleTest]]: [MySQL] 24,760 pass(es).
[ View ]
#25 drupal-HEAD-1284428012.patch259.37 KBhswong3i
PASSED: [[SimpleTest]]: [MySQL] 24,766 pass(es).
[ View ]
#22 drupal-6.x-1284382718.patch25.78 KBhswong3i
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-6.x-1284382718.patch.
[ View ]
#18 drupal_code_clean-1226500171.patch8.71 KBhswong3i
Failed: Failed to apply patch.
[ View ]
#16 drupal_code_clean-299778-16.patch19.17 KBcburschka
Failed: Failed to apply patch.
[ View ]
#14 cc-d7_cvs-1226251644.patch21.94 KBhswong3i
Failed: Failed to apply patch.
[ View ]
#13 cc-d7_cvs-1225283235.patch21.44 KBhswong3i
Failed: Failed to apply patch.
[ View ]
#12 cc-d7_cvs-1223797908.patch21.79 KBhswong3i
Failed: Failed to apply patch.
[ View ]
#8 coding-standards-299778-D7-UNSTABLE.patch111.78 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#6 coding-standards-299778-D7-UNSTABLE2.patch112.63 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#5 coding-standards-299778-D7.patch44.65 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#1 cc-d7_cvs-1223531475.patch22.31 KBhswong3i
Failed: Failed to apply patch.
[ View ]
d7-code-clean.patch69.86 KBhswong3i
Failed: Failed to apply patch.
[ View ]

Comments

hswong3i’s picture

Title:Cleanup CVS HEAD with scripts/code-clean.sh» code clean: CVS HEAD
Status:Needs review» Reviewed & tested by the community
StatusFileSize
new22.31 KB
Failed: Failed to apply patch.
[ View ]

Just coding style cleanup with scripts/code-clean.sh. Should able to be RTBC directly?

chx’s picture

In case you do not see what gets changed in the patch , click select all to highlight it. In this very nice patch we get rid of a number of trailing whitespaces. Nice job.

Dave Reid’s picture

Patch applied cleanly and always +1 for core coding consistency!

catch’s picture

Status:Reviewed & tested by the community» Needs work

No longer applies cleanly.

Dave Reid’s picture

Status:Needs work» Needs review
StatusFileSize
new44.65 KB
Failed: Failed to apply patch.
[ View ]

Re-rolled. Did searches for the following:
Replaced tabs with spaces: '\t' => ' '
Removed trailing spaces: '[ \t]+$' => ''
Checked for un-even indents (i.e. three spaces instead of two): [^( )+ [^\* ]
Found a few array-indentation & trailing comma issues as well

I should really just try and get coder working on HEAD. :)

Dave Reid’s picture

StatusFileSize
new112.63 KB
Failed: Failed to apply patch.
[ View ]

Holy crap. What we're fixing:
- Trailing spaces at end of lines
- Tabs and mis-indentations
- Space after control operators (like if, else, switch, etc)
- Space between ) and {
- STRING CONCATENATION

I started running all tests and I'm also checking through the patch line by line to make sure I didn't make any mistakes.

NOTICE: There were no kittens harmed during this patch process.

Dave Reid’s picture

All tests ran. Passed 100% (except for that damn upload test which passes for everyone else). Patch applies cleanly. No change in functionality - just coding standards. This should be ready, but webchick should have final approval.

Dave Reid’s picture

StatusFileSize
new111.78 KB
Failed: Failed to apply patch.
[ View ]

Re-rolled with the rollback of drupal_set_title stuff.

webchick’s picture

Status:Needs review» Needs work

A couple of these look a bit odd and I don't think were caught by your regex fu. From bottom to top because that's how I read huge patches liek this. :P

<?php
@@ -350,7 +350,7 @@ class UserPictureTestCase extends Drupal
   
*
    *
results: The image shouldn't be uploaded
    */
-   function testWithoutGDinvalidSize() {
+  function testWithoutGDinvalidSize() {
     if ($this->_directory_test)
       if (!image_get_toolkit()) {
         $this->drupalLogin($this->user);
?>

If we're out-denting the function, we probably need to out-dent its PHPDoc as well, no?

<?php
@@ -341,7 +341,7 @@ class UserPictureTestCase extends Drupal
        
// Check if file is not uploaded.
        
$this->assertFalse(is_file($pic_path), t('File was not uploaded.'));
       }
-   }
+  }
 
  
/**
    * Do the test:
?>

Hm. That one got outdented one level more than the parent bracket did. Mistake?

<?php
@@ -134,13 +134,13 @@ class TaxonomyVocabularyFunctionsTestCas
     $edit
['vid'] = $vid;
    
// Get data using function.
    
$getEdit = taxonomy_vocabulary_load($vid);
-    foreach(
$getEdit as $key => $value ) {
+    foreach (
$getEdit as $key => $value ) {
      
$this->assertEqual($value, $edit[$key], t('Checking value of @key.', array('@key' => $key)));
     }
?>

While we're at it, there shouldn't be a space after $value.

<?php
@@ -5,13 +5,13 @@ class TaxonomyVocabularyLoadTestCase ext
  
/**
    * Implementation of getInfo().
    */
-   function getInfo() {
-     return array(
-      
'name' => t('Loading taxonomy vocabularies'),
-      
'description' => t('Test loading vocabularies under various conditions.'),
-      
'group' => t('Taxonomy'),
-     );
-   }
+  function
getInfo() {
+    return array(
+     
'name' => t('Loading taxonomy vocabularies'),
+     
'description' => t('Test loading vocabularies under various conditions.'),
+     
'group' => t('Taxonomy'),
+    );
+  }
 
  
/**
    * Implementation of setUp() {
?>

same outdenting of PHPDoc problem here.

webchick’s picture

Ok. Didn't quite make it this time. We'll try for this at the end of UNSTABLE-3. Hopefully as separate, smaller patches that each do one thing and are easy to bang through and review. :)

Dave Reid’s picture

Status:Needs work» Postponed

Agreed. I'll be on top of my game next time! Same bat time, same bat issue queue!

hswong3i’s picture

Status:Postponed» Needs review
StatusFileSize
new21.79 KB
Failed: Failed to apply patch.
[ View ]

Should we just keep the work as simple as possible? I think cleanup with scripts/code-clean.sh is totally enough at this moment ?_?

hswong3i’s picture

Title:code clean: CVS HEAD» [Code clean] CVS HEAD
StatusFileSize
new21.44 KB
Failed: Failed to apply patch.
[ View ]

Patch reroll via CVS HEAD.

hswong3i’s picture

Title:[Code clean] CVS HEAD» [Code clean] CVS HEAD with scripts\code-clean.sh
StatusFileSize
new21.94 KB
Failed: Failed to apply patch.
[ View ]

Patch reroll via CVS HEAD.

P.S. Should we always apply scripts\code-clean.sh before commit?

cburschka’s picture

Status:Needs review» Reviewed & tested by the community

Good idea...

Also, applies and passes, so let's get this in before it breaks again.

(I hate patches that are so big they need to get re-rolled on every tiny little commit...)

cburschka’s picture

StatusFileSize
new19.17 KB
Failed: Failed to apply patch.
[ View ]

common.test was fixed already. Here's a rerun.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks! :)

hswong3i’s picture

Title:[Code clean] CVS HEAD with scripts\code-clean.sh» [Code clean] CVS HEAD with scripts/code-clean.sh
Status:Fixed» Reviewed & tested by the community
StatusFileSize
new8.71 KB
Failed: Failed to apply patch.
[ View ]

Well, patch reroll via CVS HEAD.

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

webchick’s picture

Status:Needs work» Reviewed & tested by the community

Submitting for re-testing after snafu earlier

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

hswong3i’s picture

Title:[Code clean] CVS HEAD with scripts/code-clean.sh» [Code clean] CVS with scripts/code-clean.sh
Version:7.x-dev» 6.x-dev
Status:Needs work» Needs review
StatusFileSize
new25.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-6.x-1284382718.patch.
[ View ]

A tiny fix to drupal-6.x-dev CVS with scripts/code-clean.sh

Dave Reid’s picture

Version:6.x-dev» 7.x-dev

Would need to be fixed in 7.x first.

Status:Needs review» Needs work

The last submitted patch, drupal-6.x-1284382718.patch, failed testing.

hswong3i’s picture

Status:Needs work» Needs review
StatusFileSize
new259.37 KB
PASSED: [[SimpleTest]]: [MySQL] 24,766 pass(es).
[ View ]

Patch re-roll via CVS HEAD. Run code-clean.sh directly, and additional fix with modules/system/system.tar.inc comment syntax.

chx’s picture

Status:Needs review» Needs work

modules/system/system.tar.inc the PEAR class is off limits. We are not touching it.

hswong3i’s picture

StatusFileSize
new232.98 KB
PASSED: [[SimpleTest]]: [MySQL] 24,760 pass(es).
[ View ]

Retouch the patch that remove all changes for modules/system/system.tar.inc, and nothing else from #25.

P.S. How about misc/ui/jquery.*.js? Should we also keep as untouch, or tidy with our coding syntax?

hswong3i’s picture

Status:Needs work» Needs review
moshe weitzman’s picture

please don't fork jquery ui just for whitespace and other coding standards trivia. thats silly from a maintenance point of view.

hswong3i’s picture

StatusFileSize
new32.28 KB
PASSED: [[SimpleTest]]: [MySQL] 24,791 pass(es).
[ View ]

Patch retouch from #27, re-roll with latest CVS HEAD, remove all changes to misc/ui/jquery*.

Status:Needs review» Needs work

The last submitted patch, drupal-HEAD-1284611436.patch, failed testing.

hswong3i’s picture

Status:Needs work» Needs review

#30: drupal-HEAD-1284611436.patch queued for re-testing.

lambic’s picture

Status:Needs review» Needs work

Patch doesn't apply

TravisCarden’s picture

Status:Needs work» Closed (duplicate)