Moodle
  1. Moodle
  2. MDL-30989

portfolio API, check and update DocBlock

    Details

    • Rank:
      37393

      Description

      Check and update documentation, so that it should comply with moodle coding guidelines.
      Following needs to be updated/checked for portfolio api

      1. DocBlock for page and functions.
      2. All the files should be checked/updated.

      Note: You can create sub-tasks, so as to avoid bulk integration.

        Activity

        Hide
        Sam Hemelryk added a comment -

        Hi Rosie,

        I've just been looking at this now and have made the following notes:

        1. Looks like portfolio/add.php and portfolio/file.php should be included in this portfolio API doc tidy up.
        2. All @todo and // TODO's should have an MDL issue associated with them. Please check the modified files for them and ensure they have a related tracker issue.
        3. portfoliolib.php
          • Alignment of doc block for portfolio_add_button::set_formats
          • Check @return for portfolio_add_button::get_* methods
          • Check @param for $mimetype in portfolio_instance_select
          • Check @param for $record in portfolio_instance
          • Check @return for portfolio_static_function
          • portfolio_report_insane: Comment for @param $return and update @return to reflect HTML being returned
          • portfolio_expected_time_file $totest === array|stored_file
          • portfolio_filesize_info Missing return
          • portfolio_export_pagesetup needs review $PAGE is clearly a moodle_page and $caller looks like a portfolio_caller_base instance.
          • portfolio_export_type_to_id returns string|false
        4. caller.php
          • portfolio_caller_base
            • Please check the alignment of phpdoc blocks in general
            • __construct @param is an array not an object
            • export_config_form second @param should use portfolio_plugin_base rather than object, not 100% accurate but will provide a good clue about where to look.
            • get is missing @return
            • set is missing @return
            • check @return for get_export_config... is it an array?
            • whitespace for display_name()
            • set_context $PAGE= moodle_page
          • portfolio_module_caller_base
            • whitespace for get_navigation (crazy wrapping here)

        I've stopped the review at this point Rosie, many of the things coming up are there time and time again. Please review all areas and repost for peer-review when you're ready.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Rosie, I've just been looking at this now and have made the following notes: Looks like portfolio/add.php and portfolio/file.php should be included in this portfolio API doc tidy up. All @todo and // TODO's should have an MDL issue associated with them. Please check the modified files for them and ensure they have a related tracker issue. portfoliolib.php Alignment of doc block for portfolio_add_button::set_formats Check @return for portfolio_add_button::get_* methods Check @param for $mimetype in portfolio_instance_select Check @param for $record in portfolio_instance Check @return for portfolio_static_function portfolio_report_insane: Comment for @param $return and update @return to reflect HTML being returned portfolio_expected_time_file $totest === array|stored_file portfolio_filesize_info Missing return portfolio_export_pagesetup needs review $PAGE is clearly a moodle_page and $caller looks like a portfolio_caller_base instance. portfolio_export_type_to_id returns string|false caller.php portfolio_caller_base Please check the alignment of phpdoc blocks in general __construct @param is an array not an object export_config_form second @param should use portfolio_plugin_base rather than object, not 100% accurate but will provide a good clue about where to look. get is missing @return set is missing @return check @return for get_export_config... is it an array? whitespace for display_name() set_context $PAGE= moodle_page portfolio_module_caller_base whitespace for get_navigation (crazy wrapping here) I've stopped the review at this point Rosie, many of the things coming up are there time and time again. Please review all areas and repost for peer-review when you're ready. Cheers Sam
        Hide
        Rossiani Wijaya added a comment -

        Hi Sam,

        I updated the patch according to your recommendation.

        Please let me know if it needs more changes.

        Thanks
        Rosie

        Show
        Rossiani Wijaya added a comment - Hi Sam, I updated the patch according to your recommendation. Please let me know if it needs more changes. Thanks Rosie
        Hide
        Michael de Raadt added a comment - - edited

        Some comments:

        The comments in these files is very generic (which somewhat reflects the nature of the API, but more specific information is needed to help developers who wish to use it. Where there is parameter with a generic class or array type, a list of expected fields or an example should be given, otherwise it is very unclear what needs to be passed.

        We don't know how different apps/systems will interpret the descriptions with blank lines in them, so I'd recommend descriptions on classes and functions don't contain blank lines.

        lib/portfolio/caller.php

        • There is a lack of capitalisation and punctuation that makes this hard to read.
        • The file description should be "This file contains base classes that are extended to create portfolio export functionality."
        • Line 43, comment should be "@var stdclass course active during the call" (or should that be during instantiation?)
        • Line 46, comment should be "@var array configuration used for export" and @link tags after this should be @see tags
        • Line 52, "@var object" should be "@var stdClass"
        • Line 55, "this can be overridden in subclasses constructors if they want" should be "can be optionally overridden by subclass constructors"
        • Lines 58 and 61, "stored_file|object" should be "stored_file|stdClass" - will this ever be a generic object or will it always be a file object (if so, stored_file is sufficient)?
        • Line 64, is the type of $intendedmimetype really mixed? Wouldn't it be string?
        • Line 90, what is the actual type of the parameter? Is it an array of moodle forms or is it a single moodle form? And what is the correct class name for a moodle form?
        • Remove the blank lines from the comment starting on 141. Also, this comment does not make sense. What is the function intended for?
        • Line 215, the @link should be a @see tag
        • Lines 259 and 274, remove blank comment line
        • Line 297, what does copy_existing_file() return, this should be reflected in the return type
        • The end of the description at lines 315/316 does not make sense
        • Line 464, again, can we get more specific than mixed?
        • Lines 498 and 502, remove blank comment line
        • Line 503 should use the @link tag
        • Line 513, type should be stdClass
        • Line 519, type should be stdClass (or is this an int?)
        • Line 547, remove blank comment line

        lib/portfolio/constants.php

        • The comment at line 28 should just be a normal // comment, rather than a docblock comment. Same for comments starting at 73, 147 and 176.

        lib/portfolio/exceptions.php

        • Lines 29, 94 and 106, remove blank comment line

        lib/portfolio/exporter.php

        • Lines 30, 136, 158, 197, 718, 771, 851 remove blank comment line
        • Lines 91 and 97, some inconsistent indenting
        • Lines 478, 721, "boolean" should be "bool"
        • There needs to be a space before @uses at 855

        lib/portfolio/formats.php

        • Lines 33, 128, 220, 278, 313, 336, 489, 511, 538, 564, remove blank comment line
        • Lines 56, 161, 349, 407, "in" should "from"
        • Lines 68, 173, 361, @link should be @see
        • Lines 72, 86 "informations" should be "information"
        • Lines 73, 178, 366, 429, the description of the array is ambiguous and could be more specific or refer to another function
        • Line 86, should "stored_file|object" just be "stored_file"?
        • Line 241, I'm not sure what this means
        • The comment at 481 should not be a docblock comment, just use //

        lib/portfolio/formats/leap2a/lib.php

        • Lines 30, 32, 41, 211, 353, remove blank comment line
        • Line 40, "@TODO" should be "@todo"
        • Line 124, "make an entry that has previously been added into the feed into a selection" should be "select an entry that has previously been added into the feed"
        • Lines 324, 352, should be a @link

        lib/portfolio/forms.php

        • Lines 33, 132, 255, 309, remove blank comment line
        • Lines 135, 259, should be a @link

        lib/portfolio/plugin.php

        • Line 22, should be a @link
        • Lines 34, 150, 162, 174, 209, 254, 342, 451, 568, 599, 726, 768, remove blank comment line
        • Lines 45, 57, "boolean" should be "bool"
        • Line 72, should that be "stdClass"?
        • Line 112, "pill" should be "pull"
        • Lines 320, 340, 362, 386, need to be @see links
        • The array from line 602 needs to be explained or exemplified
        • There needs to be a blank comment line after the description at 712

        lib/portfoliolib.php

        • Lines 57, 61, 68, 74, 514, 517, 549, 576, 615, 788, 983, 985, 1184, 1215, remove blank comment line
        • Lines 409, 417, 425, 433, 1042, there needs to be a blank line after the description
        • Lines 827, 858, 896, 1295, the description of the array is ambiguous and could be more specific or refer to another function
        • Line 1233, "@link" should be "@see"

        There's a lot of code in that API. I was going cross-eyed. At least the comments are colourfully written by Penny

        Show
        Michael de Raadt added a comment - - edited Some comments: The comments in these files is very generic (which somewhat reflects the nature of the API, but more specific information is needed to help developers who wish to use it. Where there is parameter with a generic class or array type, a list of expected fields or an example should be given, otherwise it is very unclear what needs to be passed. We don't know how different apps/systems will interpret the descriptions with blank lines in them, so I'd recommend descriptions on classes and functions don't contain blank lines. lib/portfolio/caller.php There is a lack of capitalisation and punctuation that makes this hard to read. The file description should be "This file contains base classes that are extended to create portfolio export functionality." Line 43, comment should be "@var stdclass course active during the call" (or should that be during instantiation?) Line 46, comment should be "@var array configuration used for export" and @link tags after this should be @see tags Line 52, "@var object" should be "@var stdClass" Line 55, "this can be overridden in subclasses constructors if they want" should be "can be optionally overridden by subclass constructors" Lines 58 and 61, "stored_file|object" should be "stored_file|stdClass" - will this ever be a generic object or will it always be a file object (if so, stored_file is sufficient)? Line 64, is the type of $intendedmimetype really mixed? Wouldn't it be string? Line 90, what is the actual type of the parameter? Is it an array of moodle forms or is it a single moodle form? And what is the correct class name for a moodle form? Remove the blank lines from the comment starting on 141. Also, this comment does not make sense. What is the function intended for? Line 215, the @link should be a @see tag Lines 259 and 274, remove blank comment line Line 297, what does copy_existing_file() return, this should be reflected in the return type The end of the description at lines 315/316 does not make sense Line 464, again, can we get more specific than mixed? Lines 498 and 502, remove blank comment line Line 503 should use the @link tag Line 513, type should be stdClass Line 519, type should be stdClass (or is this an int?) Line 547, remove blank comment line lib/portfolio/constants.php The comment at line 28 should just be a normal // comment, rather than a docblock comment. Same for comments starting at 73, 147 and 176. lib/portfolio/exceptions.php Lines 29, 94 and 106, remove blank comment line lib/portfolio/exporter.php Lines 30, 136, 158, 197, 718, 771, 851 remove blank comment line Lines 91 and 97, some inconsistent indenting Lines 478, 721, "boolean" should be "bool" There needs to be a space before @uses at 855 lib/portfolio/formats.php Lines 33, 128, 220, 278, 313, 336, 489, 511, 538, 564, remove blank comment line Lines 56, 161, 349, 407, "in" should "from" Lines 68, 173, 361, @link should be @see Lines 72, 86 "informations" should be "information" Lines 73, 178, 366, 429, the description of the array is ambiguous and could be more specific or refer to another function Line 86, should "stored_file|object" just be "stored_file"? Line 241, I'm not sure what this means The comment at 481 should not be a docblock comment, just use // lib/portfolio/formats/leap2a/lib.php Lines 30, 32, 41, 211, 353, remove blank comment line Line 40, "@TODO" should be "@todo" Line 124, "make an entry that has previously been added into the feed into a selection" should be "select an entry that has previously been added into the feed" Lines 324, 352, should be a @link lib/portfolio/forms.php Lines 33, 132, 255, 309, remove blank comment line Lines 135, 259, should be a @link lib/portfolio/plugin.php Line 22, should be a @link Lines 34, 150, 162, 174, 209, 254, 342, 451, 568, 599, 726, 768, remove blank comment line Lines 45, 57, "boolean" should be "bool" Line 72, should that be "stdClass"? Line 112, "pill" should be "pull" Lines 320, 340, 362, 386, need to be @see links The array from line 602 needs to be explained or exemplified There needs to be a blank comment line after the description at 712 lib/portfoliolib.php Lines 57, 61, 68, 74, 514, 517, 549, 576, 615, 788, 983, 985, 1184, 1215, remove blank comment line Lines 409, 417, 425, 433, 1042, there needs to be a blank line after the description Lines 827, 858, 896, 1295, the description of the array is ambiguous and could be more specific or refer to another function Line 1233, "@link" should be "@see" There's a lot of code in that API. I was going cross-eyed. At least the comments are colourfully written by Penny
        Hide
        Michael de Raadt added a comment -

        A lot of my comments above related to blank lines. According to Raj any system/editor should handle this (which sounds optimistic to me, but...). The main thing is that the presentation is consistent.

        Show
        Michael de Raadt added a comment - A lot of my comments above related to blank lines. According to Raj any system/editor should handle this (which sounds optimistic to me, but...). The main thing is that the presentation is consistent.
        Hide
        Rossiani Wijaya added a comment -

        Thanks for reviewing Michael.

        Updating patch and submitting for integration review.

        Show
        Rossiani Wijaya added a comment - Thanks for reviewing Michael. Updating patch and submitting for integration review.
        Hide
        Sam Hemelryk added a comment -

        Thanks Rosie, this has been integrated now.
        I did make one more commit however to tidy up the last few things as identified by the checker.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks Rosie, this has been integrated now. I did make one more commit however to tidy up the last few things as identified by the checker. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Pass!

        Show
        Sam Hemelryk added a comment - Pass!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks!

        Closing as fixed, heading to zzzZZZzzz, niao

        Show
        Eloy Lafuente (stronk7) added a comment - It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks! Closing as fixed, heading to zzzZZZzzz, niao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: