Hi,

I'm working on an enterprise Drupal project in the UK, and I'm currently integrating Superfish. We love the module, it's going to save us a lot of time, but where possible we like to bring stuff up to Drupal coding standards (it makes our automated tools less noisy). So I ran Superfish under the Drupal code-sniffs, and fixed a whole bunch of minor formatting errors. There are a few issues left (line-length, mostly), but I thought I'd start with the easy stuff.

CommentFileSizeAuthor
#4 superfish-coding-standards-2136511-4.patch39.11 KBdotton
#1 superfish-coding-standards-2136511-1.patch39.13 KBdotton

Comments

dotton’s picture

dotton commented
StatusFileSize
new superfish-coding-standards-2136511-1.patch39.13 KB

Patch against the head of 7.x-1.x attached.

eli-t’s picture

eli-t
he/him
Primary language English
Location Manchester
commented

Running

phpcs . --standard=Drupal --extensions=module,install,inc

before patch:

FILE: /home/vagrant/temp/superfish/superfish.admin.inc
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AFFECTING 4 LINE(S)

FILE: /home/vagrant/temp/superfish/superfish.drush.inc
--------------------------------------------------------------------------------
FOUND 11 ERROR(S) AND 3 WARNING(S) AFFECTING 9 LINE(S)

FILE: /home/vagrant/temp/superfish/superfish.install
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)

FILE: /home/vagrant/temp/superfish/superfish.module
--------------------------------------------------------------------------------
FOUND 109 ERROR(S) AND 25 WARNING(S) AFFECTING 112 LINE(S)

After patch:

FILE: /home/vagrant/temp/superfish/superfish.install
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------

FILE: /home/vagrant/temp/superfish/superfish.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------

so this looks really good.

One query, after patch, at line 1760

/**
 * Helper function that builds the nested lists of a Superfish menu.
 *
 * @param array $variables
 *   .
 *
 * @return array
 *   .
 */

I don't agree with putting the . instead of an actual description just to get rid of the warning.

Apart from that I can confirm this patch contains no functional changes.

eli-t’s picture

eli-t
he/him
Primary language English
Location Manchester
commented
Status: Needs review » Needs work
dotton’s picture

dotton commented
StatusFileSize
new superfish-coding-standards-2136511-4.patch39.11 KB

Agreed. I'm not clear on what the values in the $variables array do, so I've stripped the error-suppressing dots.

dotton’s picture

dotton commented
Status: Needs work » Needs review
eli-t’s picture

eli-t
he/him
Primary language English
Location Manchester
commented
Assigned: dotton » Unassigned
Status: Needs review » Reviewed & tested by the community

OK - I've just run PHPCS againt the new patch and two errors are returned but that's much better than artificially suppressing them.

Patch looks good.

mehrpadin’s picture

mehrpadin commented

Hey everybody,

Thanks for this :) please check the development release once updated, note I haven't exactly used the patch, but followed everything, gives me no error, so should be fine, thanks again!

eli-t’s picture

eli-t
he/him
Primary language English
Location Manchester
commented

Looks like there's still no blank line at the end of the file. Apart from that looks good in latest dev.

mehrpadin’s picture

mehrpadin commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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