Moodle
  1. Moodle
  2. MDL-30986

external API, check and update DocBlock

    Details

    • Rank:
      37390

      Description

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

          I ran your code through Marina's checker. It was mostly successful, except it pointed out that some of your parameter descriptions lacked a comment, such as course/externallib.php, line 64, $courseid. I think that's OK as $courseid is reasonably obvious, but for completeness, you might want to add simple descriptions.

          • course/externallib.php
            • line 64
          • enrol/externallib.php
            • line 360
            • line 424
            • line 670
            • line 707
          • lib/externallib.php
            • line 137
          • message/externallib.php
            • line 65
            • line 213

          There were a couple more technical problems

          • files/externallib.php, line 33, "@package core_file" should be "@package core_files" (I would never have spotted that)
          • lib/externallib.php: "No one-line description found in phpdocs for class restricted_context_exception at line 101", which I believe is incorrect

          In a visual inspection:

          • course/externallib.php
            • line 273, "see get_course_parameters()" should be an inline @see " {@see get_course_parameters()}

              "

            • lines 605, 615, 628, 640, 652, 665, 677, the @deprecated could include an MDL issue that can be used to remind us to to remove this function (in say 2.4?)
            • lines 623, 660 the description could be expanded
            • line 625, the options array could do with some description or a link - what could I pass to this?
          • enrol/externallib.php
            • Lines 471, 483, 506, 596, 621, 635... @deprecated could include an MDL issue, could be the same as the previous file

          Saving at this point...

          Show
          Michael de Raadt added a comment - I ran your code through Marina's checker. It was mostly successful, except it pointed out that some of your parameter descriptions lacked a comment, such as course/externallib.php, line 64, $courseid. I think that's OK as $courseid is reasonably obvious, but for completeness, you might want to add simple descriptions. course/externallib.php line 64 enrol/externallib.php line 360 line 424 line 670 line 707 lib/externallib.php line 137 message/externallib.php line 65 line 213 There were a couple more technical problems files/externallib.php, line 33, "@package core_file" should be "@package core_files" (I would never have spotted that) lib/externallib.php: "No one-line description found in phpdocs for class restricted_context_exception at line 101", which I believe is incorrect In a visual inspection: course/externallib.php line 273, "see get_course_parameters()" should be an inline @see " {@see get_course_parameters()} " lines 605, 615, 628, 640, 652, 665, 677, the @deprecated could include an MDL issue that can be used to remind us to to remove this function (in say 2.4?) lines 623, 660 the description could be expanded line 625, the options array could do with some description or a link - what could I pass to this? enrol/externallib.php Lines 471, 483, 506, 596, 621, 635... @deprecated could include an MDL issue, could be the same as the previous file Saving at this point...
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Mike. I'll change all, except the comments about deprecated function. I don't want to encourage anyone to use these deprecated functions. I'll still add the MDL for the deprecation reason and a @todo MDL for the reminder.

          Show
          Jérôme Mouneyrac added a comment - Thanks Mike. I'll change all, except the comments about deprecated function. I don't want to encourage anyone to use these deprecated functions. I'll still add the MDL for the deprecation reason and a @todo MDL for the reminder.
          Hide
          Jérôme Mouneyrac added a comment -

          Done, MDL-31194 has been created. I'll post in the forum about that

          Show
          Jérôme Mouneyrac added a comment - Done, MDL-31194 has been created. I'll post in the forum about that
          Hide
          Michael de Raadt added a comment -

          Hi, Jerome.

          What you've added for the deprecated functions appears correct to me, and the format, with @todo and @see tags, is correct.

          Continuing on...

          • enrol/manual/externallib.php looks good
          • files/externallib.php
            • The @params for function in this file should be described, at least the ones that haven't been deprecated
          • group/externallib.php looks good
          • lib/externallib.php looks good
          • message/externallib.php looks good
          • notes/externallib.php looks good
          • user/externallib.php looks good

          Once descriptions are added to files/externallib.php, I'm happy if this is pushed to integration.

          Show
          Michael de Raadt added a comment - Hi, Jerome. What you've added for the deprecated functions appears correct to me, and the format, with @todo and @see tags, is correct. Continuing on... enrol/manual/externallib.php looks good files/externallib.php The @params for function in this file should be described, at least the ones that haven't been deprecated group/externallib.php looks good lib/externallib.php looks good message/externallib.php looks good notes/externallib.php looks good user/externallib.php looks good Once descriptions are added to files/externallib.php, I'm happy if this is pushed to integration.
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Mike for the peer-review I made the last changes, submitting for integration.

          Show
          Jérôme Mouneyrac added a comment - Thanks Mike for the peer-review I made the last changes, submitting for integration.
          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 -

          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
          Aparup Banerjee added a comment -

          ah Jerome, this need to be rebased :-/

          Show
          Aparup Banerjee added a comment - ah Jerome, this need to be rebased :-/
          Hide
          Jérôme Mouneyrac added a comment -

          no problem, it's done

          Show
          Jérôme Mouneyrac added a comment - no problem, it's done
          Hide
          Aparup Banerjee added a comment -

          Hello,
          i am getting conflicts still: at first the conflicts were on integration.git, but i checked with moodle.git and that one todo line is still conflicting

          i've pushed up branch int_master_MDL-30986 @ git://github.com/nebgor/moodle.git with a conflict resolution (kept the todo). just mentioning it here just so that i can test with automated pre-checker for all the phpdocs changes.

          Show
          Aparup Banerjee added a comment - Hello, i am getting conflicts still: at first the conflicts were on integration.git, but i checked with moodle.git and that one todo line is still conflicting i've pushed up branch int_master_ MDL-30986 @ git://github.com/nebgor/moodle.git with a conflict resolution (kept the todo). just mentioning it here just so that i can test with automated pre-checker for all the phpdocs changes.
          Hide
          Aparup Banerjee added a comment - - edited

          stopping review here for now (some integration review issue : problem with automated checking with theres is a conflict resolution).

          Jerome if possible you may want to rebase to resolve your conflict for next week, there is a 'TODO MDL-31117' that is conflicting with master on moodle.git .
          (edit: MDL-31117 is closed - please remove the 'todo' from your patch)

          i've attached my integration branch for your ref but it will be too old next week.

          Show
          Aparup Banerjee added a comment - - edited stopping review here for now (some integration review issue : problem with automated checking with theres is a conflict resolution). Jerome if possible you may want to rebase to resolve your conflict for next week, there is a 'TODO MDL-31117 ' that is conflicting with master on moodle.git . (edit: MDL-31117 is closed - please remove the 'todo' from your patch) i've attached my integration branch for your ref but it will be too old next week.
          Hide
          Jérôme Mouneyrac added a comment -

          I created a new branch, it should be clean

          Show
          Jérôme Mouneyrac added a comment - I created a new branch, it should be clean
          Hide
          Sam Hemelryk added a comment -

          Assigning to Eloy so that he can look at this on Wednesday

          Show
          Sam Hemelryk added a comment - Assigning to Eloy so that he can look at this on Wednesday
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry, I'm reopening this because it continues failing on merge (Error: The MDL-30986-attempt2 branch at git://github.com/mouneyrac/moodle.git does not apply clean to master) in a lot of occurrences. I think its' worth solving them before landing.

          Note that you will need to wait for tomorrow's weeklies roll, so you will be 100% sure you are rebasing against the last files available.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, I'm reopening this because it continues failing on merge (Error: The MDL-30986 -attempt2 branch at git://github.com/mouneyrac/moodle.git does not apply clean to master) in a lot of occurrences. I think its' worth solving them before landing. Note that you will need to wait for tomorrow's weeklies roll, so you will be 100% sure you are rebasing against the last files available. Ciao
          Hide
          Jérôme Mouneyrac added a comment -

          Just noticing this issue. I fix new conflict and rebase. Please integrate before anything else about web service
          Cheers,
          Jerome

          Show
          Jérôme Mouneyrac added a comment - Just noticing this issue. I fix new conflict and rebase. Please integrate before anything else about web service Cheers, Jerome
          Hide
          Jérôme Mouneyrac added a comment -

          Ah wrong MDL, it is in fact MDL-30986-attempt2. Fixing it...

          Show
          Jérôme Mouneyrac added a comment - Ah wrong MDL, it is in fact MDL-30986 -attempt2. Fixing it...
          Hide
          Jérôme Mouneyrac added a comment -

          ok you can integrate MDL-30986-attempt2

          Show
          Jérôme Mouneyrac added a comment - ok you can integrate MDL-30986 -attempt2
          Hide
          Dan Poltawski added a comment -

          Stealing this off Eloy to help the merge pain

          Show
          Dan Poltawski added a comment - Stealing this off Eloy to help the merge pain
          Hide
          Dan Poltawski added a comment -

          Thanks Jerome, i've integrated this now

          Show
          Dan Poltawski added a comment - Thanks Jerome, i've integrated this now
          Hide
          Dan Poltawski added a comment -

          Passing as I ran Marinas phpdoc checker and no issues were found.

          Show
          Dan Poltawski added a comment - Passing as I ran Marinas phpdoc checker and no issues were found.
          Hide
          Jérôme Mouneyrac added a comment -

          This is great thanks a lot!

          Show
          Jérôme Mouneyrac added a comment - This is great thanks a lot!
          Hide
          Eloy Lafuente (stronk7) added a comment -
          UPDATE tracker_issues
             SET status = 'Closed',
                comment = 'Thanks!'
          WHEN participants = 'Did a gorgeous work'
          

          This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

          Show
          Eloy Lafuente (stronk7) added a comment - UPDATE tracker_issues SET status = 'Closed', comment = 'Thanks!' WHEN participants = 'Did a gorgeous work' This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

            People

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

              Dates

              • Created:
                Updated:
                Resolved: