Moodle
  1. Moodle
  2. MDL-30999

grade API, check and update DocBlock

    Details

    • Rank:
      37403

      Description

      Check and update documentation, so that it should comply with moodle coding guidelines.
      Following needs to be updated/checked for grade 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.

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment -

          Still working on this but I think lib/gradelib.php is done.

          Show
          Andrew Davis added a comment - Still working on this but I think lib/gradelib.php is done.
          Hide
          Andrew Davis added a comment - - edited

          Ive done some of the more high profile files in /grade. Not sure if I'm meant to go through every single file.

          Show
          Andrew Davis added a comment - - edited Ive done some of the more high profile files in /grade. Not sure if I'm meant to go through every single file.
          Hide
          David Mudrak added a comment -

          > Not sure if I'm meant to go through every single file.

          Hi Andrew. I had the same question and the answer was yes, we are supposed to document both libraries and all the callers.

          Show
          David Mudrak added a comment - > Not sure if I'm meant to go through every single file. Hi Andrew. I had the same question and the answer was yes, we are supposed to document both libraries and all the callers.
          Hide
          Rajesh Taneja added a comment -

          Updated bug to reflect grade, as Andrew worked on Grade API and David is working on Grading API

          Show
          Rajesh Taneja added a comment - Updated bug to reflect grade, as Andrew worked on Grade API and David is working on Grading API
          Hide
          Martin Dougiamas added a comment - - edited

          "@category grade" is everywhere ... it should only go on the bits that activity modules use, like grade_get_grades(), get_grading_manager() and grade_update(), as well as the files mod/xxx/grade.php and the gradebook callbacks in mod/xxx/lib.php

          And this is also (not coincidentally) the same things that http://docs.moodle.org/dev/Gradebook_API should be providing an overview of.

          Show
          Martin Dougiamas added a comment - - edited "@category grade" is everywhere ... it should only go on the bits that activity modules use, like grade_get_grades(), get_grading_manager() and grade_update(), as well as the files mod/xxx/grade.php and the gradebook callbacks in mod/xxx/lib.php And this is also (not coincidentally) the same things that http://docs.moodle.org/dev/Gradebook_API should be providing an overview of.
          Hide
          Michael de Raadt added a comment -

          Hi, Andrew.

          Working my way through this....

          • grade/edit/tree/calculation_form.php, line 18, "editing" is misspelt
          • grade/index.php, line 18, the file description does not make sense to me
          • grade/querylib.php
            • line 18, drop the "by"
            • lines 29, 133, @public should be "@access public"
            • lines 31, 135, the type should be int|array or mixed
            • lines 247, 278, what is the specific class for a cm? Is there one? Otherwise perhaps shift object to stdClass
            • are all the functions in this file useful parts of the API? If not, the category should be applied to specific functions instead of the whole file
          • lib/grade/grade_category.php
            • This file needs a closer look
            • Numerous instances of boolean should be changed to bool
            • The category should be applied to the class rather than the file
            • Lines 148, 153, 158, @var tags needed
            • Line 167, object to stdClass or something more specific
            • Line3 169, 1014, 1032, 1439, @static should be before the @param
            • Lines 185, 196, 207, 261, 327, 389, 769, 1248, 1285,1297, 1553, 1597, blank line after description (shift @static to immediately before @param)
            • Lines 359, 434, 525, 638, 828, 877, 1012, 1090, 1197, 1331, 1368, 1403, 1437, 1485, remove blank line before @return
            • Lines 527, 1033, @todo should have an MDL issue, which could be the current issue (or you could even remove it)
            • There's little point to is_editable()
            • Line 1642, could probably remove the @return
          • lib/grade/grade_grade.php
            • Numerous instances of boolean should be changed to bool
            • Lines 156, 199, 221, 255, 264, 274, 289, 315, 383, 432, 445, 463, 491, 502, 513, 697, 733, 746, blank line after description
          • lib/grade/grade_item.php, same with booleans and blank lines
          • lib/grade/grade_object.php, same
          • lib/grade/grade_outcome.php, same
          • lib/grade/grade_scale.php, same
          • lib/gradelib.php
            • The category should be applied individually to functions, the file seems to be separated in two for internal/public
            • You could leave the "@access public" tags in
            • grade_update(), line 60, needs a @return
            • Line 309, 869, 1004, 1170, boolean -> bool
            • Line 610, please explain "NULL means no setting==remove"

          A lot of minor little things.

          Your comment adjustments were great.

          Show
          Michael de Raadt added a comment - Hi, Andrew. Working my way through this.... grade/edit/tree/calculation_form.php, line 18, "editing" is misspelt grade/index.php, line 18, the file description does not make sense to me grade/querylib.php line 18, drop the "by" lines 29, 133, @public should be "@access public" lines 31, 135, the type should be int|array or mixed lines 247, 278, what is the specific class for a cm? Is there one? Otherwise perhaps shift object to stdClass are all the functions in this file useful parts of the API? If not, the category should be applied to specific functions instead of the whole file lib/grade/grade_category.php This file needs a closer look Numerous instances of boolean should be changed to bool The category should be applied to the class rather than the file Lines 148, 153, 158, @var tags needed Line 167, object to stdClass or something more specific Line3 169, 1014, 1032, 1439, @static should be before the @param Lines 185, 196, 207, 261, 327, 389, 769, 1248, 1285,1297, 1553, 1597, blank line after description (shift @static to immediately before @param) Lines 359, 434, 525, 638, 828, 877, 1012, 1090, 1197, 1331, 1368, 1403, 1437, 1485, remove blank line before @return Lines 527, 1033, @todo should have an MDL issue, which could be the current issue (or you could even remove it) There's little point to is_editable() Line 1642, could probably remove the @return lib/grade/grade_grade.php Numerous instances of boolean should be changed to bool Lines 156, 199, 221, 255, 264, 274, 289, 315, 383, 432, 445, 463, 491, 502, 513, 697, 733, 746, blank line after description lib/grade/grade_item.php, same with booleans and blank lines lib/grade/grade_object.php, same lib/grade/grade_outcome.php, same lib/grade/grade_scale.php, same lib/gradelib.php The category should be applied individually to functions, the file seems to be separated in two for internal/public You could leave the "@access public" tags in grade_update(), line 60, needs a @return Line 309, 869, 1004, 1170, boolean -> bool Line 610, please explain "NULL means no setting==remove" A lot of minor little things. Your comment adjustments were great.
          Hide
          Michael de Raadt added a comment -

          Are there any callbacks in standard modules or elsewhere that need to be documented as part of this API?

          Show
          Michael de Raadt added a comment - Are there any callbacks in standard modules or elsewhere that need to be documented as part of this API?
          Hide
          Andrew Davis added a comment -

          Still plodding along with this. I'll squish a lot of the commits that are currently in the branch on github.

          Show
          Andrew Davis added a comment - Still plodding along with this. I'll squish a lot of the commits that are currently in the branch on github.
          Hide
          Andrew Davis added a comment -

          I think the phpdocs are actually done. I still have to finish the wiki documentation for the grades API.

          Show
          Andrew Davis added a comment - I think the phpdocs are actually done. I still have to finish the wiki documentation for the grades API.
          Hide
          Andrew Davis added a comment -

          Ive raised MDL-31214 to remind me to come back and look at this area again. Something seems wrong about how grade_update_mod_grades() in lib/gradelib.php works.

          Show
          Andrew Davis added a comment - Ive raised MDL-31214 to remind me to come back and look at this area again. Something seems wrong about how grade_update_mod_grades() in lib/gradelib.php works.
          Hide
          Andrew Davis added a comment -

          http://docs.moodle.org/dev/Gradebook_API is almost finished. I just need to add an example of how to use outcomes.

          Show
          Andrew Davis added a comment - http://docs.moodle.org/dev/Gradebook_API is almost finished. I just need to add an example of how to use outcomes.
          Hide
          Andrew Davis added a comment -

          Im still working on an example use of outcomes for the wiki docs. It would be nice to get the php docs looked at while Im doing that.

          Show
          Andrew Davis added a comment - Im still working on an example use of outcomes for the wiki docs. It would be nice to get the php docs looked at while Im doing that.
          Hide
          Andrew Davis added a comment -

          I think http://docs.moodle.org/dev/Gradebook_API is actually done.

          Show
          Andrew Davis added a comment - I think http://docs.moodle.org/dev/Gradebook_API is actually done.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          I'm sending this back sorry - needs more work.
          Unfortunately I only have a couple of hours at work today before I head off on holiday and didn't get to this till 30 minutes ago, and 40 minutes before I head off.
          I've started the review anyway to give you an idea where it is heading.

          lib/grade/constants.php

          • defines should have phpdoc blocks

          lib/grade_category.php

          • grade_category::build_path appears to return a string not an int
          • grade_category::fetch should use @return grade_category (has an unwanted object in there)
          • @private is redundant on private methods. It's not bad just raising it although I do think we should be consistent within a closest parent, so all private methods in a class have it or none.
          • grade_category::sum_grades @param for $grade shouldn't have an & in docblock
          • other functions with the same thing
          • grade_category::load_grade_item returns grade_object, but get_grade_item which it uses returns grade_item?
          • grade_category::fetch_course_category same thing as ::fetch.

          lib/grade_grade.php

          • grade_grade class missing phpdoc block with copyright, license and package
          • grade_item property, is that likely to be a grade_item object?
          • timecreated property has a todo that doesn't have an associated MDL tracker issue
          • timemodified same thing
          • fetch_users_grades $grade_item again likely to be a grade_item instance?
          • load_grade_item should user @return grade_item, unwanted object there
          • get_dategraded method has a todo without MDL issue number
          • fetch the same as load_grade_item
          • incorrect @params for get_hiding_affected
          • more @param object $grade_item … will stop mentioning all should be reviewed
          • insert has a todo without tracker issue
          • notify_changed typo in @param

          grade_item.php

          • grade_item class missing @license and copyright
          • item_category and parent_category objects use @var object, both have comments describing the class that is actually being used (other properties as well please review all)
          • $formula property missing @var… $dependson_cache as well
          • load_outcome @return has unwanted object instead of proper class
          • As above but load_parent_category
          • @return fetch_course_item is wrong
          • grade_item::get_final has todo without mdl tracker issue number
          • move_after_sortorder missing doc block
          • update_final_grade incorrect @params
          • update_raw_grade incorrect @params
          • compute missing @param
          • use_formula missing @params, @return etc
          • validate_formula incorrect @param
          • get_coefstring missing @param

          I only got as far as those four files sorry.
          A couple of general notes

          • TODO's need to have MDL issue numbers associated with them
          • The object class was deprecated in 2.0 and using @param object, or @return object always signifies something is wrong
          • Classes need to have at least @package, @copyright, @license
          • Marina has a checker for phpdocs that is really useful. Its still a work in progress and it misses lots of things still but in general it is pretty good (just turn off the rule about comments with more than 2 //) as part of the integration we run that as a reference.
          • Using @static, @private, @protected for method docblocks is redundant if the keyword is present in the definition. It's permitted to put it in still but should be consistent across the class, or file.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, I'm sending this back sorry - needs more work. Unfortunately I only have a couple of hours at work today before I head off on holiday and didn't get to this till 30 minutes ago, and 40 minutes before I head off. I've started the review anyway to give you an idea where it is heading. lib/grade/constants.php defines should have phpdoc blocks lib/grade_category.php grade_category::build_path appears to return a string not an int grade_category::fetch should use @return grade_category (has an unwanted object in there) @private is redundant on private methods. It's not bad just raising it although I do think we should be consistent within a closest parent, so all private methods in a class have it or none. grade_category::sum_grades @param for $grade shouldn't have an & in docblock other functions with the same thing grade_category::load_grade_item returns grade_object, but get_grade_item which it uses returns grade_item? grade_category::fetch_course_category same thing as ::fetch. lib/grade_grade.php grade_grade class missing phpdoc block with copyright, license and package grade_item property, is that likely to be a grade_item object? timecreated property has a todo that doesn't have an associated MDL tracker issue timemodified same thing fetch_users_grades $grade_item again likely to be a grade_item instance? load_grade_item should user @return grade_item, unwanted object there get_dategraded method has a todo without MDL issue number fetch the same as load_grade_item incorrect @params for get_hiding_affected more @param object $grade_item … will stop mentioning all should be reviewed insert has a todo without tracker issue notify_changed typo in @param grade_item.php grade_item class missing @license and copyright item_category and parent_category objects use @var object, both have comments describing the class that is actually being used (other properties as well please review all) $formula property missing @var… $dependson_cache as well load_outcome @return has unwanted object instead of proper class As above but load_parent_category @return fetch_course_item is wrong grade_item::get_final has todo without mdl tracker issue number move_after_sortorder missing doc block update_final_grade incorrect @params update_raw_grade incorrect @params compute missing @param use_formula missing @params, @return etc validate_formula incorrect @param get_coefstring missing @param I only got as far as those four files sorry. A couple of general notes TODO's need to have MDL issue numbers associated with them The object class was deprecated in 2.0 and using @param object, or @return object always signifies something is wrong Classes need to have at least @package, @copyright, @license Marina has a checker for phpdocs that is really useful. Its still a work in progress and it misses lots of things still but in general it is pretty good (just turn off the rule about comments with more than 2 //) as part of the integration we run that as a reference. Using @static, @private, @protected for method docblocks is redundant if the keyword is present in the definition. It's permitted to put it in still but should be consistent across the class, or file. Cheers Sam
          Hide
          Andrew Davis added a comment - - edited

          Im still working through this. I have linked the various MDLs I've created along the way.

          Show
          Andrew Davis added a comment - - edited Im still working through this. I have linked the various MDLs I've created along the way.
          Hide
          Andrew Davis added a comment -

          I think Im done. Thanks for the feedback Sam and Marinas tool was also really helpful. I believe I've done all of the external surfaces of the grades API. Marina's script still turns up approximately a bazillion issues within the gradebook internals

          Show
          Andrew Davis added a comment - I think Im done. Thanks for the feedback Sam and Marinas tool was also really helpful. I believe I've done all of the external surfaces of the grades API. Marina's script still turns up approximately a bazillion issues within the gradebook internals
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Some little things:

          • When one file is part of one plugin, the package must be that plugin and not any subsystem. You've done that properly in modules, they have package = mod_xxx and then, category = grade (the API). But in reports, you've set package = core_grades and that's not valid. They must be package = gradereport_xxxx always, and then category if part of the API and subpackage in case you want to perform some sort of subdivision.
          • The phpdocs checker says that glossary_grade_item_update() has incomplete parameters list.
          • Less important but there are a bunch of comments that have a wrong alignment (number of spaces before comments):
          • lib/grade/grade_object.php (#436)
          • lib/grade/grade_item.php (class grade_item docs)
          • lib/grade/grade_item.php (public function get_parent_category())
          • lib/grade/grade_item.php (public function get_item_category())
          • lib/grade/grade_category.php (#1509 - #1513)
          • grade/querylib.php (#32 has whitespace and #33 missing asterisk

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Some little things: When one file is part of one plugin, the package must be that plugin and not any subsystem. You've done that properly in modules, they have package = mod_xxx and then, category = grade (the API). But in reports, you've set package = core_grades and that's not valid. They must be package = gradereport_xxxx always, and then category if part of the API and subpackage in case you want to perform some sort of subdivision. The phpdocs checker says that glossary_grade_item_update() has incomplete parameters list. Less important but there are a bunch of comments that have a wrong alignment (number of spaces before comments): lib/grade/grade_object.php (#436) lib/grade/grade_item.php (class grade_item docs) lib/grade/grade_item.php (public function get_parent_category()) lib/grade/grade_item.php (public function get_item_category()) lib/grade/grade_category.php (#1509 - #1513) grade/querylib.php (#32 has whitespace and #33 missing asterisk Ciao
          Hide
          Andrew Davis added a comment -

          Ok, that's all done. I also cleaned up some other bits and pieces I noticed. The diff grows ever larger...

          Show
          Andrew Davis added a comment - Ok, that's all done. I also cleaned up some other bits and pieces I noticed. The diff grows ever larger...
          Hide
          Andrew Davis added a comment -

          Should I squash all of these commits or would that mess up the pending integration? Let me know if you would like me to do it or feel free to do it yourself using your integrator magic

          Show
          Andrew Davis added a comment - Should I squash all of these commits or would that mess up the pending integration? Let me know if you would like me to do it or feel free to do it yourself using your integrator magic
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, 2 more little things and yes, please, I think it's better to squash all these.

          • There are 5 occurrences of the @access tag (lib/gradelib.php has 3 and grade/querylib.php has 2). I detected it because your patch affected one of them in querylib. Perhaps it would be a good moment to take rid of them (have not much sense under PHP5).
          • There are still some @package tags incorrect. I think that, while we can ignore the rest of still pending errors (3 slashes everywhere, some functions missing some @params, other incorrect packages...), at least we must make the @package tags correct for the gradebook ones, so here they are:
            • Package gradereport is not valid: That's not one valid frankenstyle component, hence is not a valid package at all. Surely if nothing else matches (subsystem...) it should be, simply, "core". It happens @ grade/report/index.php , grade/report/lib.php
            • Package gradebook is not valid: grade/report/grader/lib.php, grade/report/lib.php, grade/report/overview/lib.php, grade/report/user/lib.php
            • Package gradereport_grader is not valid: It's being used by other gradereports (gradereport_outcomes, gradereport_user). Each one should have its own package.

          And that is, I'd happy to integrate this once fixed. As said, those files are still plenty of phpdocs-errors but I don't think it's time to fix all them now.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, 2 more little things and yes, please, I think it's better to squash all these. There are 5 occurrences of the @access tag (lib/gradelib.php has 3 and grade/querylib.php has 2). I detected it because your patch affected one of them in querylib. Perhaps it would be a good moment to take rid of them (have not much sense under PHP5). There are still some @package tags incorrect. I think that, while we can ignore the rest of still pending errors (3 slashes everywhere, some functions missing some @params, other incorrect packages...), at least we must make the @package tags correct for the gradebook ones, so here they are: Package gradereport is not valid: That's not one valid frankenstyle component, hence is not a valid package at all. Surely if nothing else matches (subsystem...) it should be, simply, "core". It happens @ grade/report/index.php , grade/report/lib.php Package gradebook is not valid: grade/report/grader/lib.php, grade/report/lib.php, grade/report/overview/lib.php, grade/report/user/lib.php Package gradereport_grader is not valid: It's being used by other gradereports (gradereport_outcomes, gradereport_user). Each one should have its own package. And that is, I'd happy to integrate this once fixed. As said, those files are still plenty of phpdocs-errors but I don't think it's time to fix all them now. TIA and ciao
          Hide
          Andrew Davis added a comment -

          I've squashed the commits and removed all instances of @access.

          I think I have the @packages correct. For /grade/report/lib.php and /grade/report/lib.php I set their package to core_grades. Given they're within the grades directory is felt weird setting them to core.

          Show
          Andrew Davis added a comment - I've squashed the commits and removed all instances of @access. I think I have the @packages correct. For /grade/report/lib.php and /grade/report/lib.php I set their package to core_grades. Given they're within the grades directory is felt weird setting them to core.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well, I agree "core" is weird (or at least nor very informative)...

          But packages can only be VALID COMPONENT names. And that excludes everything not being:

          1) One valid frankenstyle plugin (plugintype_pluginname).
          2) One valid subsystem (from get_core_subsystems() ).
          3) "core"

          And both "gradereport" and "gradebook" ARE not any of 1, 2 and 3. Hence they are incorrect.

          Plz, share it with Raj, he knows because we had some discussions last week related to other issues.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, I agree "core" is weird (or at least nor very informative)... But packages can only be VALID COMPONENT names. And that excludes everything not being: 1) One valid frankenstyle plugin (plugintype_pluginname). 2) One valid subsystem (from get_core_subsystems() ). 3) "core" And both "gradereport" and "gradebook" ARE not any of 1, 2 and 3. Hence they are incorrect. Plz, share it with Raj, he knows because we had some discussions last week related to other issues. Ciao
          Hide
          Andrew Davis added a comment -

          But I'm not using "gradebook" or plain "gradereport" anywhere /grade/report/lib.php and and /grade/report/lib.php are currently marked as core_grades.

          Show
          Andrew Davis added a comment - But I'm not using "gradebook" or plain "gradereport" anywhere /grade/report/lib.php and and /grade/report/lib.php are currently marked as core_grades.
          Hide
          Rajesh Taneja added a comment -

          Hello Andrew,

          Please have a look at all the files under grader (grade/report/grader/*.php)
          These are not valid packages.

          Show
          Rajesh Taneja added a comment - Hello Andrew, Please have a look at all the files under grader (grade/report/grader/*.php) These are not valid packages.
          Hide
          Andrew Davis added a comment -

          but gradereport is a valid plugin type. Its listed in get_plugin_types() and the grade reports are plugins.

          Show
          Andrew Davis added a comment - but gradereport is a valid plugin type. Its listed in get_plugin_types() and the grade reports are plugins.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Aha, it is seems you finally got that plugin types ARE NOT valid component names, hence, they cannot be used as @package.

          Also, I've added one extra commit replacing some incorrect

          {@see}

          tags by

          {@link}

          .

          So, this has been integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Aha, it is seems you finally got that plugin types ARE NOT valid component names, hence, they cannot be used as @package. Also, I've added one extra commit replacing some incorrect {@see} tags by {@link} . So, this has been integrated, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Nobody tested this, passing.

          Show
          Eloy Lafuente (stronk7) added a comment - Nobody tested this, passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some changes to Moodle should be milestones in the project by themselves.

          This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Some changes to Moodle should be milestones in the project by themselves. This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: