Comments

shadcn’s picture

Status: Active » Needs review
StatusFileSize
new4.09 KB
pfrenssen’s picture

Title: Fixed some typos and wording in apps.api.php » Fix typos, wording and coding standards in apps.api.php
Status: Needs review » Needs work

Great job, but it needs some more work. I have only reviewed the first function, but these remarks apply to the entire page. I'd also want to extend the scope of this issue to also make sure this page conforms to coding standards.

/*
 * Provides configuration information to the apps about an enabled app
 *
 * One can register demo content as a separate module or as a set of callbacks
 * hook_apps_app_info is a hook that will only be called on app modules
 *
 * RETURN: an associative array
 */
  • Incorrect commenting style, should have two asterisks on the first line.
  • @file is missing.
  • hooks_apps_info should be followed by ().
  • Sentences should end in a period.
  • Use correct doxygen formatted documentation for the @return value.
function hook_apps_app_info() {                                                 
  return array(                                                                 
    //Demo Content                                                              
    'demo content description' => 'This tells what add demo content will do it is placed on the configure form',
    //The preferred way for an app to provide demo content is to have a module  
    //that when enabled will add demo content, and when disabled will removed   
    //demo content                                                              
    //this module should be a submodule or part of the manifest dependent modules
    'demo content module' => 'appname_demo_content',                            
                                                                                
    //If the demo content is provided in a different way one should provide the 
    //following callbacks                                                       
    'demo content enabled' => 'appname_demo_content_enabled', //should return True if demo content is on
    'demo content enable' => 'appname_demo_content_enable', //should turn on demo content and return true
    'demo content disable' => 'appname_demo_content_disable', //should turn off demo content and return true
                                                                                
    'configure form' => 'appname_app_configure_form', // This form will be render on the app config page
    'post install callback' => 'appname_app_post_install', // This will be called after the app is enabled initially or when the app has been uninstalled
    'status callback' => 'appname_app_status'                                   
    /*                                                                          
    This callback is used to render a status table on the config page. It should be an array with two keys (and on optional third) 
    array(                                                                      
      'title' =>'Status'  //title of the table,                                 
      'items' => array(  //rows in the table with any keys                      
        array(                                                                  
          'severity' =>    REQUIREMENT_WARNING, //REQUIREMENT_OK REQUIREMENT_INFO, REQUIREMENT_ERROR
          'title' => 'Example',                                                 
          'description' => t("Instructions for Example"),                       
          'action' => array(l("Link to do something!", "")),                    
        ),                                                                      
      ),                                                                        
      // headers are optional but these are the default                         
      'headers' => array('severity', 'title', 'description', 'action')          
    );                                                                          
    serverity and                                                               
    */                                                                          
}
  • All comments that start with // should be preceded with a space.
  • All sentences should end with a period.
  • Do not use inline comments (eg. "should return True...")
  • All comments should start with a capital letter.
  • Comments should never exceed 80 characters.
  • TRUE and FALSE should always be capitalized.
  • No comments in the style /** */ may be used inside functions.
  • Make sure there is no trailing whitespace.
  • Properties of an array that are printed on a separate line should always end on a comma.
  • Use single quotes instead of double quotes where appropriate.
  • The code is invalid due to a missing closing bracket.
  • Remove the words "serverity and" at the end, this makes no sense.
babruix’s picture

StatusFileSize
new43.91 KB

Fixed found issues, in some places documentation still doesn`t have @param description.
This patch fixes many coding standarts issues and added/fixed documentation.

babruix’s picture

Status: Needs work » Needs review
pfrenssen’s picture

This is already much better, but there still remain some problems. When I examine the file with the Coder module:

[pieter@archlinux apps (7.x-1.x *%=)]$ drush dcs apps.api.php 

FILE: ./apps/apps.api.php
--------------------------------------------------------------------------------
FOUND 57 ERROR(S) AND 12 WARNING(S) AFFECTING 41 LINE(S)
--------------------------------------------------------------------------------
pfrenssen’s picture

Status: Needs review » Needs work
StatusFileSize
new45.49 KB

Did some more cleanups. Some work still remaining:

[pieter@archlinux apps (7.x-1.x *%=)]$ drush dcs apps.api.php 

FILE: ./apps/apps.api.php
--------------------------------------------------------------------------------
FOUND 21 ERROR(S) AND 2 WARNING(S) AFFECTING 19 LINE(S)
--------------------------------------------------------------------------------
  44 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
  45 | WARNING | Line exceeds 80 characters; contains 131 characters
  45 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
  45 | ERROR   | Whitespace found at end of line
  46 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
  47 | ERROR   | Line indented incorrectly; expected 2 spaces, found 6
  48 | ERROR   | Line indented incorrectly; expected 2 spaces, found 6
  49 | ERROR   | Line indented incorrectly; expected 2 spaces, found 8
  50 | WARNING | Line exceeds 80 characters; contains 101 characters
  50 | ERROR   | Line indented incorrectly; expected 2 spaces, found 10
  51 | ERROR   | Line indented incorrectly; expected 2 spaces, found 10
  52 | ERROR   | Line indented incorrectly; expected 2 spaces, found 10
  53 | ERROR   | Line indented incorrectly; expected 2 spaces, found 10
  54 | ERROR   | Line indented incorrectly; expected 2 spaces, found 8
  55 | ERROR   | Line indented incorrectly; expected 2 spaces, found 6
  56 | ERROR   | Line indented incorrectly; expected 2 spaces, found 6
  57 | ERROR   | Line indented incorrectly; expected 2 spaces, found 6
  58 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
  90 | ERROR   | Parameter comment indentation must be 2 additional spaces at
     |         | position 1
  93 | ERROR   | Return comment indentation must be 2 additional spaces
  97 | ERROR   | "require_once" is a statement not a function; no parentheses
     |         | are required
  97 | ERROR   | Language constructs must be followed by a single space;
     |         | expected "require_once (" but found "require_once("
 105 | ERROR   | Empty array declaration must have no space between the
     |         | parentheses
--------------------------------------------------------------------------------
babruix’s picture

Status: Needs work » Needs review
StatusFileSize
new46.07 KB

Found issues fixed, please review.

mglaman’s picture

Issue summary: View changes
Status: Needs review » Needs work

Really need this. However does not apply against latest dev. re-roll incoming.

mglaman’s picture

Title: Fix typos, wording and coding standards in apps.api.php » Fix typos and review coding standards.
StatusFileSize
new49.77 KB

Began review with Coder.

Was "Coder found 1 projects, 21 files, 7 critical warnings, 52 normal warnings, 500 minor warnings, 0 warnings were flagged to be ignored"

Now "Coder found 1 projects, 21 files, 7 critical warnings, 43 normal warnings, 250 minor warnings, 0 warnings were flagged to be ignored"

Still needs work. Also updated title.

  • hefox committed 40ba324 on 7.x-1.x
    Issue #1875766: by arshadcn, pfrenssen, babruix, mglaman: fix coding...
hefox’s picture

Status: Needs work » Active

patch defiently an improvment so commited as is

  • hefox committed 9d1ec97 on 7.x-1.x
    Issue #1875766: fix up more code style issues
    
hefox’s picture

Got a few more, but think it needs a manual once over and there's some @todo about function description

  • hefox committed 40ba324 on 7.x-1.x-1411912
    Issue #1875766: by arshadcn, pfrenssen, babruix, mglaman: fix coding...
  • hefox committed 9d1ec97 on 7.x-1.x-1411912
    Issue #1875766: fix up more code style issues