Task description
This task is to ensure that all functions, classes, methods, and constants in the FAQ and Avatar Selection modules have properly formed, complete Doxygen code comments.

Doxygen is the documentation generation system that Drupal uses. The documentation is extracted directly from the sources, which makes it much easier to keep the documentation consistent with the source code. The documentation should contain information that is helpful to developers who may not be familiar with the code, such as explaining what functions do, how functions should be called and what information the different parameters contain. Developers often need need to check the API documentation when creating patches or developing new features and so it is important to have clear, correct documentation.

To complete this task, review the two modules and add API documentation to any function, class, method, or constant that does not have it or expand on existing documentation where it is incomplete, unclear, or does not follow established Drupal standards. See the resources section for more information, particularly the "Drupal Doxygen formatting conventions".

The final deliverable will be a set of 2 patches against the 6.x versions of the FAQ and Avatar Selection modules posted to the issue queue that has been reviewed and marked "Ready to be Committed".

Resources

Primary contact
Stella Power (http://drupal.org/user/66894 - snpower)

Cheers,
Stella

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aclight’s picture

Title: GHOP #xx: Add PHP documentation to FAQ and Avatar Selection modules » GHOP #97: Add PHP documentation to FAQ and Avatar Selection modules
stella’s picture

Claimed by daryennaruto

stella’s picture

Claimed by andrei.ruse

andrei.ruse’s picture

Assigned: Unassigned » andrei.ruse
FileSize
13.26 KB

I have attached the 2 files, zipped, faq.module and faq.install, with the documentation done as much as I could.
I hope everything is alright. I will try to post the other 2 files, avatar_selection.module and avatar_selection.install in 24 hours, in order you to review it until 4th of February.

aclight’s picture

@andrei.ruse: Make sure that you create patches for your changes and don't post the full files themselves, as specified in the task description.

Check out the patch creation handbook page at http://drupal.org/patch/create for more information. Or, you can drop by the #drupal-ghop or #drupal IRC channels on irc.freenode.net and one of us can guide you through the creation process.

stella’s picture

FYI: I've a rather unfortunately timed trip this weekend. I'm unavailable from 7pm GMT this evening to 7pm GMT Sunday (3rd Feb) for reviewing patches. The deadline is 0hrs PST on the 4th Feb, so I will be around to do reviewing before then. I'll also be available on the #drupal-ghop channel if you want to talk over anything.

Cheers,
Stella

stella’s picture

I've reviewed the files, though you do need to create patches as aclight said.

faq.install

  • You need to add a @file block. See the Doxygen page for more details, or have a look at another project that does this correctly, e.g. coder.
  • All comments must begin with a capital letter and end with valid punctuation, e.g. a period / full-stop. For example, line 5 is missing a period, line 33 doesn't begin with a capital letter, while line 61 is missing both.
  • For function descriptions, the description for the @return value should be indented on the following line and shouldn't be present on the same line as the @return. This is incorrect on line 6.
  • Description for faq_update_1() should be something more like "Create the new faq_weights table when upgrading older installations."

faq.module

  • I found a couple of tabs in the file, please use spaces instead. See the coding-standards doc for more info.
  • Again you need to add a @file block. See the Doxygen page for more details, or have a look at another project that does this correctly, e.g. coder.
  • All comments must begin with a capital letter and end with valid punctuation, e.g. a period / full-stop.
  • Don't forget that all inline comments, that already existed before you started, need to be changed so they begin with a capital letter and end with a period.
  • faq_help() function: The formatting and details are incorrect, but since it is an implementation of hook_help you just need to say
    /**
     * Implementation of hook_help().
     */
    

    . Similarly with faq_perm(), faq_access(), faq_menu(), faq_menu_alter() and faq_theme().

  • faq_node_info() function: Incorrect details and formatting of @return. That function defines the 'faq' node / content type.
  • A lot of functions have invalidly formatted @return lines. The description should be indented on the following line and not on the same line as the @return. There's too many affected functions to list here.
  • The @param lines should be separated from the function descriptions by a blank line.
  • Comment lines should be no more than 80 chars long, so where you have a description that goes over this, e.g. for faq_general_settings_form(), then change the format to be like:
    /**
     * Define a form to edit the page header and descriptive text, which can be
     * shown as html via the drupal_get_form() function.
     * ... etc
     */
    
  • faq_node_name(): invalid formatting. It's enough to just say "Implementation of hook_node_name()."
  • faq_delete(): a more correct description is that it deletes a faq node from the database.
  • faq_view(): a more correct description is that it displays a faq node, rather than an entry from the FAQ. The @return description doesn't make sense, just say it returns the node object.
  • faq_settings_page(): missing @param lines.
  • faq_general_settings_form(): you don't need to add in the drupal_get_form() bit.
  • faq_questions_settings_form(), faq_categories_settings_form(): for @return, just say it returns the $form array. You're over-complicating it.
  • For $form_state @param lines, it's enough to say it stores the form values submitted. Again you're over-complicating it.
  • faq_page(): $tid is not initially set to 0, that's the default value to use if one isn't set.
  • _display_faq_by_category(): invalid descriptions for the $category_display and $term @params. They don't "set" if those items will be displayed or not. $category_display contains which category layout should be used, while $term contains the category / term information.

Note: indenting of my example comments above don't appear correctly because of the browser display, but there should be one space before the * and one after. For @param, etc, descriptions, they should be indented by 2 spaces - that means there are actually 3 spaces between the * and the description.

There are other function descriptions left to review which I will get to shortly. The 6.x version of the coder module is documented quite well if you need an example to compare against while I'm not around.

Overall, the documentation is looking quite good. Keep up the good work!

Cheers,
Stella

stella’s picture

faq.module (cont)

  • @return line - also I don't think it's necessary to include the variable name returned here, so @return $data would be incorrect.
  • theme_questions_top(), etc: for the $result @param you have the description "Hold the FAQ entries, which will be taken from the database and shown". It might be better to say that it is a database result object which holds a list of FAQ node ids to display.
  • For the $class @param to the theme_category.....() functions, it might be good to add in that a special class name is required in order to hide the questions / answers.
  • theme_hide_answer(): the whole paragraph on $display_vars['teaser'] etc doesn't really need to be there. You can omit that.
  • theme_new_page(): For the @return description, it's enough to say "Return the html-formatted content." like you did for the other functions.
  • faq_block(): the description here is for the wrong function. It's enough to say "Implementation of hook_block()." along with listing the three blocks provided (Random FAQs, etc).
  • theme_faq_highlights(): the description would be better as "Create the html output for the Recent FAQs block."
  • theme_faq_random_highlights(): the description would be better as "Create the html output for the Random FAQs block."
  • _get_indented_faq_terms(): the description is for the wrong function and "Vocabulaire" is not a word I think. The function basically returns a html formatted list of terms indented according to the term depth.
  • faq_get_terms(): the description doesn't really describe the function. You just need to say something like it returns a list of terms associated with faq nodes. Again it's enough to say "Return the html-formatted content." for the @return description.
  • faq_get_faq_list(): the description doesn't really describe the function. It actually returns the output for the faq_site_map() function. It returns a list of FAQ categories if categorization is enabled, otherwise it returns a list of faq nodes.
  • faq_site_map(): It actually returns a list of FAQ categories if categorization is enabled, otherwise it returns a list of faq nodes. Again the @return description is overly complicated.

I'm away now until 7pm GMT on Sunday and so won't be able to review your next patches before then. I think if you can fix all of the things mentioned in this and my previous comment, and ensure you don't have similar issues in your avatar_selection patches, then there probably won't be too much to fix on Sunday evening. I'm also sure that there will be other reviewers able to help you this weekend. If you do run into problems or have any questions, I recommend that you join the #drupal-ghop IRC channel. See How to use IRC for more details on how to connect, etc.

Good luck with the rest of the task and I look forward to reviewing the rest of your patches when I return on Sunday!

Cheers,
Stella

Cheers,
Stella

andrei.ruse’s picture

Status: Active » Needs review
FileSize
21.45 KB
32.13 KB

I have attached here 2 zip files, one with the modified files, and another one with the patches, as required.
The patches seem a little too big, but since I have never used diff before, I am giving them to you as diff gave them to me:D
I hope everything is OK.

agentrickard’s picture

Status: Needs review » Needs work

The comments look good but the patches and the files are malformed.

Please format your text files with UNIX linebreaks. These are all DOS-encoded, which is improper.

That explains why your patches are so large. The extra line breaks force the diff to not recognize the original file correctly.

agentrickard’s picture

FAQ module review:

Line 5 is not needed.

* The FAQ module file, which contains all the functions related to it.

Typo on line 1040-1041.

 *   CSS class which the html div will be using. A special class name is required in order to hide
 &   questions / answers.

Remove tabs in the description of faq_block().

Remove apostrophes from FAQs, line 1936.

In general, this is great work.

agentrickard’s picture

Avatar Selection review.

Line 5 is not needed:

* The Avatar Selection module file, which contains all the functions related to it.

avatar_selection_help() should mention hook_help().

avatar_selection_access() description is hard to follow and does not define $op or $node.

Report the bug in avatar_selection_menu(), please. (Bonus point for that if you know what the bug is!)

Documentation of avatar_selection_handler_filter_role() is wrong. This does not create any roles.

The work on FAQ is better, but these are all easy to fix.

stella’s picture

Hi,

Back now. I've had a chance to review your work and it's looking very good.

faq.install:

  • You don't need line 6.
  • Line 10 refers to the table being created as the 'sql_weights' table, whereas it's actually called 'faq_weights'.

faq.module:

  • Description for faq_menu_alter() says it's an implementation of hook_access() which is incorrect.
  • faq_theme(): Just say it's an implementation of hook_theme() instead.
  • _get_indented_faq_terms(): the description is for the wrong function. A more accurate description would be that it returns a html formatted list of terms indented according to the term depth.

I'll write up a review on the Avatar Selection changes shortly.

Cheers,
Stella

andrei.ruse’s picture

Status: Needs work » Active

A couple of questions:
1. faq_settings_page()
* @param $aid // unknown usability

2. _get_indented_faq_terms()
* @param $vid
* Vocab id. - what else than vocabulary?

3. function faq_menu_alter(&$callbacks) - is it an implementation of hook_menu()?

stella’s picture

Status: Active » Needs work

avatar_selection.install

  • Line 6 is not needed.
  • avatar_selection_install(): The description is incorrect - the table structure is written to the database regardless of the 'user_pictures' setting. If it's not set, then a warning is printed, but the table is still created.

avatar_selection.module

  • You just need to say "Implementation of hook_help()." for the avatar_selection_help() function.
  • Typo on line 505 (structure spelt incorrectly).
  • Description of avatar_selection_handler_filter_role() is wrong. This does not create any roles, rather it returns a list of the available roles.

It's looking very good. Just fix these few minor errors and submit new patches before midnight (PST)!

Thanks for all your hard work on this task.

Cheers,
Stella

stella’s picture

Hi andrei.ruse!

Answers below.

1. faq_settings_page()
* @param $aid // unknown usability
>> this is actually a deprecated / obsolete variable which shouldn't really be there any more. Just say it's a deprecated variable and I'll remove it from the code later this week. Thanks for spotting this!

2. _get_indented_faq_terms()
* @param $vid
* Vocab id. - what else than vocabulary?
>> Your description is correct, it is a vocabulary id.

3. function faq_menu_alter(&$callbacks) - is it an implementation of hook_menu()?
>> No, it is an implementation of hook_menu_alter().

I'm also on the #drupal-ghop IRC channel if you prefer to ask questions there, but here is fine too.

:)
Stella

andrei.ruse’s picture

Status: Needs work » Needs review
FileSize
8.29 KB
21.4 KB

Attached:
AvatarSelection_FAQ_files.zip (the 4 files,with the code) - In case the patches are not be ok
AvatarSelection_FAQ_patches.zip (the 2 patches - much smaller than last ones)

Still I have to say that I am not a Linux user (only sometimes, from live distros), and the code was edited in Ms Frontpage & Notepad. That's why the code was nou UNIX-like.
I hope everything is ok with the patches.

stella’s picture

Status: Needs review » Reviewed & tested by the community

That's perfect! Thank you for all your hard work. All that's left for you to do is to upload the files to the GHOP task on the google tracker - http://code.google.com/p/google-highly-open-participation-drupal/issues/...

Thanks again!

Cheers,
Stella

stella’s picture

Status: Reviewed & tested by the community » Fixed

Patches submitted to CVS. Thanks again.

Cheers,
Stella

Anonymous’s picture

Status: Fixed » Closed (fixed)

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

stella’s picture

FAQ module changes released in FAQ 5.x-2.7 and FAQ 6.x-1.3.

Cheers,
Stella