Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#7 | xmlrpc_example_coding_standard_issues_2012_10_15.patch | 17.51 KB | amitgoyal |
xmlrpc_example_coding_standard_issues.patch | 22.13 KB | amitgoyal | |
Comments
Comment #1
sanchi.girotra CreditAttribution: sanchi.girotra commentedI applied this patch locally on my system and it worked perfectly fine.
Patch looks fine.
Comment #2
Mile23Looks 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:
But it does pass tests, so... Maybe rfay can weigh in.
Comment #3
rfay/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.
Comment #4
amitgoyal CreditAttribution: amitgoyal commented@rfay / @Mile23 - Not sure if anything else needs to be done here. Can you please apply this patch?
Comment #5
Mile23@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. :-)
Comment #6
Mile23Ugh. Forgot to change the status. :-)
Comment #7
amitgoyal CreditAttribution: amitgoyal commented@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.
Comment #8
sanchi.girotra CreditAttribution: sanchi.girotra commentedI tested the updated patch, it worked fine and also modified as suggested in #5.
Comment #9
Mile23Good deal. Committed: http://drupalcode.org/project/examples.git/commitdiff/70b318fd592ea84dd1...
Thanks, amitgoyal and sanchi. :-)
Comment #10
rfay@amitgoyal++
@sanchi.girotra++
@Mile23++
Thanks so much for all your work on this, everybody.
@Mile23, so wonderful to see your care for this.
Comment #11
amitgoyal CreditAttribution: amitgoyal commentedThanks a lot @Mile23 and @rfay for applying the patch!!
I will check issues in other modules as well and provide patches.
Comment #12
Mile23@amitgoya: Thanks!