Improve test coverage and function docs: context

1. +++ b/includes/context.inc
   @@ -270,6 +270,11 @@ function ctools_context_filter($contexts, $required) {
   +/**
   

   This could do with some comments

2. +++ b/includes/context.inc
   @@ -270,6 +270,11 @@ function ctools_context_filter($contexts, $required) {
   + * @param $contexts
   

   What type of parameters are these? int, array bool etc? Ps include in comment.

3. +++ b/includes/context.inc
   @@ -311,6 +316,13 @@ function ctools_context_selector($contexts, $required, $default) {
   +/**
   

   This could do with some comments

4. +++ b/includes/context.inc
   @@ -311,6 +316,13 @@ function ctools_context_selector($contexts, $required, $default) {
   + * @param $required
   

   What type of parameters are these? int, array bool etc? Ps include in comment.

5. +++ b/includes/context.inc
   @@ -703,11 +715,15 @@ function ctools_context_keyword_substitute($string, $keywords, $contexts, $conve
   +  // If not set or 'falsy'.
   

   Perhaps FALSE? Instead of falsy ;-)

6. +++ b/tests/ctools.test
   @@ -0,0 +1,235 @@
   +
   

   Needs @file tag

A few observations:

1. +++ b/ctools.api.php
   @@ -229,6 +232,28 @@ function hook_ctools_entity_context_alter(&$plugin, &$entity, $plugin_id) {
   + *   The current context plugin object. If this implemented a 'convert' function,
   

   Sorry mega trivial, but goes over 80 char limit

2. +++ b/ctools.module
   @@ -528,6 +553,8 @@ function ctools_class_add($classes, $hook = 'html') {
   +    // TODO: consider using explode(' ', $classes);
   +    // TODO: consider checking that $classes is a string before adding.
   

   Should be @todo

3. +++ b/includes/context.inc
   @@ -28,27 +28,27 @@
   +
   

   This needs a docblock.

4. +++ b/includes/context.inc
   @@ -28,27 +28,27 @@
   +  public function is_type($type) {
   

   Please add a docblock

5. +++ b/includes/context.inc
   @@ -58,30 +58,30 @@ class ctools_context {
   +  public function get_argument() {
   

   Dock block and others below

6. +++ b/includes/context.inc
   @@ -270,6 +272,21 @@ function ctools_context_filter($contexts, $required) {
   + * @return array
   

   May be worth adding comment covering whats returned in this array?

7. +++ b/includes/context.inc
   @@ -398,7 +446,8 @@ function ctools_context_converter_selector($contexts, $required, $default) {
   +      $result[] = _ctools_context_converter_selector($contexts, $r, $dv, $count++);
   

   Could $r be refactored to be more descriptive?

8. +++ b/includes/context.inc
   @@ -547,17 +622,29 @@ function ctools_context_convert_context($context, $converter, $converter_options
   +      // TODO: What's the diff btw following and "empty($r)" ?
   

   Us @todo instead of //TODO

9. +++ b/includes/context.inc
   @@ -703,11 +820,15 @@ function ctools_context_keyword_substitute($string, $keywords, $contexts, $conve
   +  // TODO: is '' the appropriate default value?
   

   @todo

10. +++ b/includes/context.inc
    @@ -715,10 +836,13 @@ function ctools_context_id($context, $type = 'context') {
      *   A list of context descriptor objects, i.e, arguments, relationships, contexts, etc.
    

    Humm? Not covered by your patch, this line runs over 80 char limit

11. +++ b/includes/context.inc
    @@ -729,9 +853,10 @@ function ctools_context_next_id($objects, $name) {
    +      // TODO: When obj has no 'id', should we increment local id? $id = $id + 1;
    

    @todo

12. +++ b/includes/context.inc
    @@ -823,6 +948,10 @@ function ctools_context_get_context_from_argument($argument, $arg, $empty = FALS
    + * @return array
    

    Please add comments for return value and @param if possible

Looks good, just a few small changes:

1. +++ b/includes/context.inc
   @@ -397,8 +592,11 @@ function ctools_context_converter_selector($contexts, $required, $default) {
   +    foreach ($required as $id => $dependency) {
   +      $dv = isset($default[$id]) ? $default[$id] : '';
   +      $result[] = _ctools_context_converter_selector(
   +        $contexts, $dependency, $dv, $count++
   +      );
   

   It'd be good to rename the $dv variable to something easily understandable.

2. +++ b/includes/context.inc
   @@ -547,22 +793,35 @@ function ctools_context_convert_context($context, $converter, $converter_options
   +      // @todo What's the difference between the following and "empty($r)" ?
   

   $r variable has now been renamed to $item, so it'd be good to update the comment to match.

Few! this is a long patch :-/ Notes below

1. +++ b/includes/context.inc
   @@ -28,28 +27,104 @@
   +   * @var
   

   This is missing a variable type and desription?

2. +++ b/includes/context.inc
   @@ -28,28 +27,104 @@
   +   * @var
   

   Missing var type and description

3. +++ b/includes/context.inc
   @@ -28,28 +27,104 @@
   +    /* Other vars are NULL. */
   

   This should use inline commented syntax //

4. +++ b/includes/context.inc
   @@ -58,30 +133,63 @@ class ctools_context {
   +   * @return null
   

   Missing method description.

5. +++ b/includes/context.inc
   @@ -91,35 +199,56 @@ class ctools_context {
   +   * Keyword strings associated with the context.
   

   needs a couple of spaces before Keyword

6. +++ b/includes/context.inc
   @@ -167,7 +325,28 @@ class ctools_context_required {
   +   * context list, there is a good chance that what happened is our context
   

   Perhaps: is the context

7. +++ b/includes/context.inc
   @@ -177,11 +356,12 @@ class ctools_context_required {
   +    // context list, there is a good chance that what happened is our context
   

   perhaps: is the context

8. +++ b/includes/context.inc
   @@ -203,24 +383,70 @@ class ctools_context_required {
   +   *   The matching ctools_context, or False if no such context was found.
   

   Perhaps lowercase or full caps for false?

9. +++ b/includes/context.inc
   @@ -1126,9 +1550,13 @@ function ctools_context_get_context_from_context($context, $type = 'context', $a
   + *   If True, placeholders are acceptable.
   

   Perhaps if TRUE

10. +++ b/plugins/contexts/token.inc
    @@ -46,7 +46,9 @@ function ctools_context_token_convert_list() {
    + * @see ctools_context_convert_context().
    

    Missing parameter types and comments

Looks good

Whew, long indeed! Committed.

This broke ctools_context_optional, opened #2905885: ctools_context_optional regression with public $required property.

This broke the Feeds tests on PHP 5.3 because the short array syntax is used. Opened #2942581: ctools.test breaks Feeds tests on PHP 5.3 because of short array syntax.