Support from Acquia helps fund testing for Drupal Acquia logo

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
FileSize
22.31 KB

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
FileSize
44.65 KB

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

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

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

@@ -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?

@@ -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?

@@ -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.

@@ -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
FileSize
21.79 KB

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
FileSize
21.44 KB

Patch reroll via CVS HEAD.

hswong3i’s picture

Title: [Code clean] CVS HEAD » [Code clean] CVS HEAD with scripts\code-clean.sh
FileSize
21.94 KB

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

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
FileSize
8.71 KB

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
FileSize
25.78 KB

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
FileSize
259.37 KB

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

FileSize
232.98 KB

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

FileSize
32.28 KB

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)