Moodle
  1. Moodle
  2. MDL-30998

grading API, check and update DocBlock

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.2.1
    • Fix Version/s: 2.5
    • Component/s: Documentation, Gradebook
    • Labels:
    • Rank:
      37402

      Description

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

        Hi Rajesh, can you please explain me what API does this subtask refer to? I noticed that Andrew has "grading" API assigned and I have "grade". However, looking at http://docs.moodle.org/dev/Core_APIs#mod you can see that the Advanced grading API (the one I was working on) is "grading" and the gradebook interaction (which Andrew was working on) is "grade". You may want to fix the subtasks titles to follow the correct API names.

        Show
        David Mudrak added a comment - Hi Rajesh, can you please explain me what API does this subtask refer to? I noticed that Andrew has "grading" API assigned and I have "grade". However, looking at http://docs.moodle.org/dev/Core_APIs#mod you can see that the Advanced grading API (the one I was working on) is "grading" and the gradebook interaction (which Andrew was working on) is "grade". You may want to fix the subtasks titles to follow the correct API names.
        Hide
        Rajesh Taneja added a comment -

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

        Show
        Rajesh Taneja added a comment - Updated bug to reflect grading, as David is working on Grading and Andrew is working on Grade API
        Hide
        Rajesh Taneja added a comment -

        Thanks for pointing that David,
        It's updated.

        Show
        Rajesh Taneja added a comment - Thanks for pointing that David, It's updated.
        Hide
        David Mudrak added a comment -

        Assigning to Marina, the component leader, as discussed. Thanks a lot.

        Show
        David Mudrak added a comment - Assigning to Marina, the component leader, as discussed. Thanks a lot.
        Hide
        Marina Glancy added a comment -

        The advanced grading/rubric component is actually 'Grading methods' and not 'Gradebook'

        Show
        Marina Glancy added a comment - The advanced grading/rubric component is actually 'Grading methods' and not 'Gradebook'
        Hide
        Michael de Raadt added a comment -

        Some comments...

        In backup/moodle2/backup_gradingform_plugin.class.php, the link at line 30 - if that is an internal link to code, it should be a @see - if it's a link to documentation, it should be a URL. Similar for backup/moodle2/restore_gradingform_plugin.class.php, grade/grading/form/lib.php, grade/grading/form/rubric/backup/moodle2/restore_gradingform_rubric_plugin.class.php.

        In grade/grading/form/lib.php:

        • Line 33, "definition" should be "definitions"
        • Line 40, there should be a comma after functions
        • I'm not sure the change at line 557 is necessary
        • Line 635, "of student using" should be "of a student, using"
        • Line 636, "in DB" should be "in the DB"
        • Line 639, "that teacher" -> "that a teacher"
        • Line 642, "not completed" -> "incomplete"
        • Line 643, "being" -> "is"
        • Line 645, should "updated" be "deleted"?
        • Line 650, "to advanced" -> "to an advanced"
        • Line 651, "into grading form so", "in the grading form, so"

        In grade/grading/form/rubric/lib.php:

        • If the todo issue at line 340 is closed, the todo can be removed
        • The change at line 558 is probably not necessary
        • Line 631, should link to a definition of the two array elements expected

        In grade/grading/form/rubric/renderer.php:

        • Lines 51, 53, 144, 237, 279, 344 and 345 could link to a description of the values expected
        • The inline @see at line 278 should be enclosed in curly braces.

        In grade/grading/form/rubric/rubriceditor.php:

        • Line 24, "that processes sort, add, delete buttons on client." doesn't make sense to me.
        • Comment starting line 36 should be "If Javascript is disabled when one of those special buttons is pressed, the form element is not validated and, instead of submitting the form, we process button presses."
        • Type at line 46 should be "string|bool"
        • The changes at lines 73 and 645 are probably not necessary as part of this issue

        A good tidy-up of the docs there. Well done.

        Show
        Michael de Raadt added a comment - Some comments... In backup/moodle2/backup_gradingform_plugin.class.php, the link at line 30 - if that is an internal link to code, it should be a @see - if it's a link to documentation, it should be a URL. Similar for backup/moodle2/restore_gradingform_plugin.class.php, grade/grading/form/lib.php, grade/grading/form/rubric/backup/moodle2/restore_gradingform_rubric_plugin.class.php. In grade/grading/form/lib.php: Line 33, "definition" should be "definitions" Line 40, there should be a comma after functions I'm not sure the change at line 557 is necessary Line 635, "of student using" should be "of a student, using" Line 636, "in DB" should be "in the DB" Line 639, "that teacher" -> "that a teacher" Line 642, "not completed" -> "incomplete" Line 643, "being" -> "is" Line 645, should "updated" be "deleted"? Line 650, "to advanced" -> "to an advanced" Line 651, "into grading form so", "in the grading form, so" In grade/grading/form/rubric/lib.php: If the todo issue at line 340 is closed, the todo can be removed The change at line 558 is probably not necessary Line 631, should link to a definition of the two array elements expected In grade/grading/form/rubric/renderer.php: Lines 51, 53, 144, 237, 279, 344 and 345 could link to a description of the values expected The inline @see at line 278 should be enclosed in curly braces. In grade/grading/form/rubric/rubriceditor.php: Line 24, "that processes sort, add, delete buttons on client." doesn't make sense to me. Comment starting line 36 should be "If Javascript is disabled when one of those special buttons is pressed, the form element is not validated and, instead of submitting the form, we process button presses." Type at line 46 should be "string|bool" The changes at lines 73 and 645 are probably not necessary as part of this issue A good tidy-up of the docs there. Well done.
        Hide
        Marina Glancy added a comment -

        Michael, thanks for feedback. Sorry did not come back to it earlier

        inline

        {@link classname}

        is permitted, did not change anything here

        grade/grading/form/rubric/lib.php, line 631 (now line 659): those elements are just arrays, nothing to link at

        the same in grade/grading/form/rubric/renderer.php:
        Lines 51, 53, 144, 237, 279, 344 and 345

        • I could only link to default options for options, but criteria is just an array, nothing to link to
        Show
        Marina Glancy added a comment - Michael, thanks for feedback. Sorry did not come back to it earlier inline {@link classname} is permitted, did not change anything here grade/grading/form/rubric/lib.php, line 631 (now line 659): those elements are just arrays, nothing to link at the same in grade/grading/form/rubric/renderer.php: Lines 51, 53, 144, 237, 279, 344 and 345 I could only link to default options for options, but criteria is just an array, nothing to link to
        Hide
        Michael de Raadt added a comment -

        Hi, Marina.

        The inline @link tag seems to have been added since I read that, so I'm OK with its use.

        With the arrays used as parameters, if you cannot link to a description of the values expected, perhaps you could give an example that covers the possible array elements. I'm thinking from the perspective of a new developer who would not know what to pass to the function when calling it.

        I'll have another quick look over the code and the API doc related to this.

        Show
        Michael de Raadt added a comment - Hi, Marina. The inline @link tag seems to have been added since I read that, so I'm OK with its use. With the arrays used as parameters, if you cannot link to a description of the values expected, perhaps you could give an example that covers the possible array elements. I'm thinking from the perspective of a new developer who would not know what to pass to the function when calling it. I'll have another quick look over the code and the API doc related to this.
        Hide
        Michael de Raadt added a comment -

        Hi, Marina.

        The changes you have made look good.

        The new comment in at line 32 of grade/grading/form/rubric/rubriceditor.php could be better worded as...

        The rubric editor is defined as a separate form element. This allows us to render criteria, levels and buttons using the rubric's own renderer. Also, the required Javascript library is included, which processes, on the client, buttons needed for reordering, adding and deleting criteria.

        I'll have a look at the docs page. Have a read through that to make sure I haven't changed the meaning of any text there.

        Show
        Michael de Raadt added a comment - Hi, Marina. The changes you have made look good. The new comment in at line 32 of grade/grading/form/rubric/rubriceditor.php could be better worded as... The rubric editor is defined as a separate form element. This allows us to render criteria, levels and buttons using the rubric's own renderer. Also, the required Javascript library is included, which processes, on the client, buttons needed for reordering, adding and deleting criteria. I'll have a look at the docs page. Have a read through that to make sure I haven't changed the meaning of any text there.
        Hide
        Michael de Raadt added a comment -

        I've made some changes to the doc, most of which are grammatical.

        I have some questions relating to the doc.

        • Will you be adding content to the Functions section?
        • With the example from the assignment module, the code is in lib.php. Where will it be for normal modules?
        • In point two of your example, you say MODNAME_grading_areas_list() should be defined. What should it do? Perhaps you could include a code example for this.
        • Where should the code in point three be located? Is that what is needed in MODNAME_grading_areas_list()? Also, where should the following examples be placed?

        I tried to override the assignment_supports() function in my own assignment type, but I was not able to disable advanced grading. I might be doing this incorrectly, I'll have to get you to check, otherwise it may be a bug.

        Show
        Michael de Raadt added a comment - I've made some changes to the doc, most of which are grammatical. I have some questions relating to the doc. Will you be adding content to the Functions section? With the example from the assignment module, the code is in lib.php. Where will it be for normal modules? In point two of your example, you say MODNAME_grading_areas_list() should be defined. What should it do? Perhaps you could include a code example for this. Where should the code in point three be located? Is that what is needed in MODNAME_grading_areas_list()? Also, where should the following examples be placed? I tried to override the assignment_supports() function in my own assignment type, but I was not able to disable advanced grading. I might be doing this incorrectly, I'll have to get you to check, otherwise it may be a bug.
        Hide
        Marina Glancy added a comment -

        Michael, I changed comment but really don't think I can do better on documenting arrays in renderer. Those arrays are as they are returned from plugin. If somebody wants to rewrite renderer, it is quite clear from the code which fields are expected in arrays.

        I will need to come back to dev docs

        Show
        Marina Glancy added a comment - Michael, I changed comment but really don't think I can do better on documenting arrays in renderer. Those arrays are as they are returned from plugin. If somebody wants to rewrite renderer, it is quite clear from the code which fields are expected in arrays. I will need to come back to dev docs
        Hide
        Sam Hemelryk 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
        Sam Hemelryk 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
        Aparup Banerjee added a comment -

        Hi Marina,
        here's my review notes on this:

        • Classes have doc blocks which are missing @package : If and when including optional doc blocks, we must still cater to the proper doc block format. ( ref: http://docs.moodle.org/dev/Coding_style#Classes_3 )
          Or you can choose to simply have the entire description in the file's doc block.

        The whole point is that a doc block exists, it must contain its full contextual information and not rely on another doc block as they can be shown separately.
        In the case of a single file that is a single class, it is optional to provide a class doc block for the case where in future, futher classes may be added to the file (imo).

        aside from above, all else looks good

        Cheers,
        Aparup

        ps: you may want to run spell checker on comments made or on a generated documents.

        Show
        Aparup Banerjee added a comment - Hi Marina, here's my review notes on this: https://github.com/marinaglancy/moodle/compare/master...wip-MDL-30998-master#L2R393 We cannot allow below statements. Can we please move the TODO outside of the code and onto a separate line and resolve this "comment" code. if ( false ){ ref : http://docs.moodle.org/dev/Coding_style#Using_TODO Classes have doc blocks which are missing @package : If and when including optional doc blocks, we must still cater to the proper doc block format. ( ref: http://docs.moodle.org/dev/Coding_style#Classes_3 ) Or you can choose to simply have the entire description in the file's doc block. following on from the above point, when ever @category is specified, we MUST have @package as we are highlighting a part of our core api (@package). (ref: http://docs.moodle.org/dev/Coding_style#.40category ) The whole point is that a doc block exists, it must contain its full contextual information and not rely on another doc block as they can be shown separately. In the case of a single file that is a single class, it is optional to provide a class doc block for the case where in future, futher classes may be added to the file (imo). aside from above, all else looks good Cheers, Aparup ps: you may want to run spell checker on comments made or on a generated documents.
        Hide
        Aparup Banerjee added a comment -

        reopening this for Marina's action.

        Show
        Aparup Banerjee added a comment - reopening this for Marina's action.
        Hide
        Michael de Raadt added a comment -

        Hi, Marina.

        It would be good if we could wrap up this issue. It is the last remaining issue in the Docs sprint.

        Show
        Michael de Raadt added a comment - Hi, Marina. It would be good if we could wrap up this issue. It is the last remaining issue in the Docs sprint.
        Hide
        Marina Glancy added a comment -

        Apu, I made changes.
        The function get_grading_manager() does not have @package and has only @category. But functions always inherit package from the file afaik
        Marina

        Show
        Marina Glancy added a comment - Apu, I made changes. The function get_grading_manager() does not have @package and has only @category. But functions always inherit package from the file afaik Marina
        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
        Dan Poltawski added a comment -

        Integrated to master only.

        Show
        Dan Poltawski added a comment - Integrated to master only.
        Hide
        Andrew Davis added a comment -

        This has been thoroughly tested. Passing.

        Show
        Andrew Davis added a comment - This has been thoroughly tested. Passing.
        Hide
        Dan Poltawski added a comment -

        Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

        Show
        Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: