There's currently a lot of code in the dev version that doesn't comply with the Drupal coding standards. That's bad for many reasons and eg. makes patching harder.

Running the module through Coder and similar to make sure there's whitespace where there should be and no whitespace where it shouldn't be would be a great start - going through the module manually would be great as well.

Let's gather patches here.

CommentFileSizeAuthor
#8 1027890-6.x-coding-standards.patch67.3 KBygerasimov
#6 1027890-coding-standards-review-2.patch67.71 KBygerasimov
#5 1027890-coding-standards-review.patch66.89 KBygerasimov
#1 1027890-coding-standards.patch66.48 KBygerasimov

Comments

ygerasimov’s picture

ygerasimov commented
Status: Active » Needs review
StatusFileSize
new 1027890-coding-standards.patch66.48 KB

Please find attached patch with a log of fixes. I haven't touched libraries (spyc.php, mimeparse.php, becode.php).

kylebrowning’s picture

kylebrowning commented

This either needs to happen RIGHT now, or before we launch, otherwise this patch will never apply cleanly since were constantly updating code

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented

I think I would rather reroll the little patches as we encounter them rather than reroll this one at the end, so I'm reviewing this one now. Need to get my CVS environments killed and rebuilt with git.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented

I still haven't applied and tested this, but here are some nitpicks I found from visual inspection.

+++ services.installundefined
@@ -5,7 +5,7 @@
+ * Implementshook_schema().

@@ -88,7 +88,7 @@ function services_schema() {
+ * Implementshook_uninstall().

Needs a space after 'Implements' in both places

+++ tests/functional/ServicesEndpointTests.testundefined
@@ -11,105 +11,106 @@
+              'administer content types',

+++ tests/functional/ServicesEndpointTests.testundefined
@@ -11,105 +11,106 @@
+              'administer content types',

+++ tests/functional/ServicesEndpointTests.testundefined
@@ -11,105 +11,106 @@
+              'administer content types',

Seems like these are indented too far

Otherwise this looks fanatastic. I want to apply it and run tests but otherwise I am really appreciative of this patch. Thank you!

Powered by Dreditor.

ygerasimov’s picture

ygerasimov commented
StatusFileSize
new 1027890-coding-standards-review.patch66.89 KB

Thank you for the review. Here is changed patch.

ygerasimov’s picture

ygerasimov commented
StatusFileSize
new 1027890-coding-standards-review-2.patch67.71 KB

Here is reroll of the patch against current master.

kylebrowning’s picture

kylebrowning commented
Version: 7.x-3.x-dev » 6.x-3.x-dev
Status: Needs review » Patch (to be ported)

Commited to 7.x needs 6.x patch

ygerasimov’s picture

ygerasimov commented
StatusFileSize
new 1027890-6.x-coding-standards.patch67.3 KB

Here is patch for 6.x-3.x.

ygerasimov’s picture

ygerasimov commented
Status: Patch (to be ported) » Needs review
kylebrowning’s picture

kylebrowning commented
Status: Needs review » Closed (fixed)

This has been committed, thanks!