Problem/Motivation

I've done a DCS check on the module file and found a few coding standard issue.

Proposed resolution

Fix issues

Remaining tasks

Create a patch file to address issues

User interface changes

none

API changes

none

Data model changes

none

CommentFileSizeAuthor
#9 webform-coding_standards-2843307-8-D7.patch1.98 KBLiam Morland
#7 dcs_in_module-2843307-7-D7.patch48.82 KBdarrenwh
#2 dcs_in_module-2843307-2-D7.patch58.64 KBdarrenwh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

darrenwh created an issue. See original summary.

darrenwh’s picture

darrenwh CreditAttribution: darrenwh commented
Status: Active » Needs review
FileSize
dcs_in_module-2843307-2-D7.patch58.64 KB

Made a start on file, up to line circa 3500

Status: Needs review » Needs work

The last submitted patch, 2: dcs_in_module-2843307-2-D7.patch, failed testing.

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at University of Waterloo commented

It appears to be making some changes which shouldn't happen:

- * $return int
+ *   $return int

Should be @return int.

-    // Only add FOR UPDATE when incrementing.
+      // Only add FOR UPDATE when incrementing.
darrenwh’s picture

darrenwh CreditAttribution: darrenwh commented

$return int was there already? the code has jus been indented...

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at University of Waterloo commented

It should have been @return all along.

darrenwh’s picture

darrenwh CreditAttribution: darrenwh commented
Status: Needs work » Needs review
FileSize
dcs_in_module-2843307-7-D7.patch48.82 KB

OK rectified that and reverted some sections where formater broke some lines

  • Liam Morland committed 54fd853 on 7.x-4.x 
    Issue #2843307 by darrenwh, Liam Morland: Coding standards
Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at University of Waterloo commented
Status: Needs review » Active
FileSize
webform-coding_standards-2843307-8-D7.patch1.98 KB

I made a big coding standards commit. Will you be continuing with this?

I did not make the changes in the attached patch. Removing those calls to t() might mess up how some people are doing translation. If we were to make these changes, it would be in a separate issue because these go beyond coding standards.

darrenwh’s picture

darrenwh CreditAttribution: darrenwh commented

In the function specification for t() https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7.x it clearly says You should never use t() to translate variables, so I'm happy to leave this change out if a new ticket can be raised off the back of it. Will checkout latest verson of the module file and finish it off in the next few days, thanks

Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at University of Waterloo commented
Status: Active » Fixed

OK, I have already committed the first batch, so please make child tickets for follow-ups. Thanks very much for your work on this.

Status: Fixed » Closed (fixed)

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