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

        Rajesh Taneja created issue -
        Rajesh Taneja made changes -
        Field Original Value New Value
        Link This issue has a clone MDL-30988 [ MDL-30988 ]
        Rajesh Taneja made changes -
        Description Check and update documentation, so that it should comply with [moodle coding guidelines|http://docs.moodle.org/dev/Coding_style#Files].
        Following needs to be updated/checked for preference api
        # DocBlock for page and functions.
        # All the files should be checked/updated.

        Note: You can create sub-tasks, so as to avoid bulk integration.
        Check and update documentation, so that it should comply with [moodle coding guidelines|http://docs.moodle.org/dev/Coding_style#Files].
        Following needs to be updated/checked for portfolio api
        # DocBlock for page and functions.
        # All the files should be checked/updated.

        Note: You can create sub-tasks, so as to avoid bulk integration.
        Component/s Portfolio API [ 10307 ]
        Component/s Administration [ 10050 ]
        Rajesh Taneja made changes -
        Link This issue has a clone MDL-30988 [ MDL-30988 ]
        Rajesh Taneja made changes -
        Link This issue is a clone of MDL-30990 [ MDL-30990 ]
        Rajesh Taneja made changes -
        Link This issue is a clone of MDL-30990 [ MDL-30990 ]
        Rajesh Taneja made changes -
        Component/s Documentation [ 10073 ]
        Rajesh Taneja made changes -
        Assignee Rajesh Taneja [ rajeshtaneja ] moodle.com [ moodle.com ]
        Michael de Raadt made changes -
        Affects Version/s STABLE backlog [ 10463 ]
        Affects Version/s 2.2 [ 10656 ]
        Affects Version/s 2.3 [ 10657 ]
        Labels triaged
        Fix Version/s 2.2.1 [ 11456 ]
        Fix Version/s 2.2 [ 10656 ]
        Michael de Raadt made changes -
        Assignee moodle.com [ moodle.com ] Rossiani Wijaya [ rwijaya ]
        Michael de Raadt made changes -
        Affects Version/s Docs Sprint [ 11551 ]
        Affects Version/s STABLE backlog [ 10463 ]
        Michael de Raadt made changes -
        Affects Version/s 2.2 [ 10656 ]
        Affects Version/s 2.2.1 [ 11456 ]
        Affects Version/s Docs Sprint [ 11551 ]
        Fix Version/s Docs Sprint [ 11551 ]
        Fix Version/s 2.2 [ 10656 ]
        Fix Version/s 2.2.1 [ 11456 ]
        Rossiani Wijaya made changes -
        Status Open [ 1 ] Development in progress [ 3 ]
        Rajesh Taneja made changes -
        Parent MDL-30971 [ 50000 ]
        Issue Type Sub-task [ 5 ] Task [ 3 ]
        Rossiani Wijaya made changes -
        Rossiani Wijaya made changes -
        Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
        Sam Hemelryk made changes -
        Original Estimate 0 minutes [ 0 ]
        Remaining Estimate 0 minutes [ 0 ]
        Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
        Peer reviewer samhemelryk
        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
        Sam Hemelryk made changes -
        Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
        Rossiani Wijaya made changes -
        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
        Rossiani Wijaya made changes -
        Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
        Michael de Raadt made changes -
        Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
        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
        Michael de Raadt made changes -
        Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
        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.
        Rossiani Wijaya made changes -
        Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
        Eloy Lafuente (stronk7) made changes -
        Currently in integration Yes [ 10041 ]
        Sam Hemelryk made changes -
        Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
        Integrator samhemelryk
        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
        Sam Hemelryk made changes -
        Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
        Fix Version/s 2.3 [ 10657 ]
        Sam Hemelryk made changes -
        Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
        Tester samhemelryk
        Hide
        Sam Hemelryk added a comment -

        Pass!

        Show
        Sam Hemelryk added a comment - Pass!
        Sam Hemelryk made changes -
        Status Testing in progress [ 10011 ] Tested [ 10006 ]
        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
        Eloy Lafuente (stronk7) made changes -
        Status Tested [ 10006 ] Closed [ 6 ]
        Resolution Fixed [ 1 ]
        Currently in integration Yes [ 10041 ]
        Eloy Lafuente (stronk7) made changes -
        Integration date 17/Feb/12
        Eloy Lafuente (stronk7) made changes -
        Fix Version/s Docs Sprint [ 11551 ]

          People

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

            Dates

            • Created:
              Updated:
              Resolved: