Please find attached a patch to fix 80+ coding standard issues in XMLRPC Example module. I have found these issues using pareview.sh script provided by klausi.

Please review and apply this patch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sanchi.girotra’s picture

Status: Needs review » Reviewed & tested by the community

I applied this patch locally on my system and it worked perfectly fine.

Patch looks fine.

Mile23’s picture

Looks like pareview.sh is a cool toolchain, and thanks for the effort.

Changes like these make me itch, however, because you're never quite sure what you're losing:

 class XmlrpcExampleTestCase extends DrupalWebTestCase {
-  protected $xmlrpc_url;
+  protected $xmlrpcUrl;

But it does pass tests, so... Maybe rfay can weigh in.

rfay’s picture

/me finds it odd that the tool would need to change a variable name. However, I note that this is done consistently, and it's only in the test. You sure have good eyes, @Mile23. Hope they're not itching.

amitgoyal’s picture

@rfay / @Mile23 - Not sure if anything else needs to be done here. Can you please apply this patch?

Mile23’s picture

@rfay: I think the premise with pareview.sh is to change things to a coding convention. So apparently there's a convention where camelCase is better.

@amitgoyal: I'm not sure there's really a convention that says camelCase is better. :-) Also, changing a protected property name is really not cool at all. It might not matter in this case (I seriously doubt that anyone is subclassing this example test case), but it could turn into a big deal pretty fast in other circumstances. Maybe the tool could add a TODO about the variable name instead of changing it, ideally with a link to the best-practices page about it.

I like the line length wrapping and the @param and @return cleanup, but we shouldn't specify a type, since this is for Drupal 7: http://drupal.org/node/1354#functions

Drupal 8 docs apparently want the type to be declared.

Thanks, and give it another try. :-)

Mile23’s picture

Status: Reviewed & tested by the community » Needs work

Ugh. Forgot to change the status. :-)

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
17.51 KB

@Mile23 - Attaching here updated patch where I have ignored the issues like camelCase and type for @param / @return.

Please review and let me know if there are any more changes required.

sanchi.girotra’s picture

Status: Needs review » Reviewed & tested by the community

I tested the updated patch, it worked fine and also modified as suggested in #5.

Mile23’s picture

Status: Reviewed & tested by the community » Fixed

Good deal. Committed: http://drupalcode.org/project/examples.git/commitdiff/70b318fd592ea84dd1...

Thanks, amitgoyal and sanchi. :-)

rfay’s picture

@amitgoyal++
@sanchi.girotra++
@Mile23++

Thanks so much for all your work on this, everybody.

@Mile23, so wonderful to see your care for this.

amitgoyal’s picture

Thanks a lot @Mile23 and @rfay for applying the patch!!

I will check issues in other modules as well and provide patches.

Mile23’s picture

@amitgoya: Thanks!

Status: Fixed » Closed (fixed)

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