Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30986

external API, check and update DocBlock

    Details

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore 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
            salvetore 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
            jerome 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
            jerome 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
            jerome Jérôme Mouneyrac added a comment -

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

            Show
            jerome Jérôme Mouneyrac added a comment - Done, MDL-31194 has been created. I'll post in the forum about that
            Hide
            salvetore 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
            salvetore 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
            jerome Jérôme Mouneyrac added a comment -

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

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

            ah Jerome, this need to be rebased :-/

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

            no problem, it's done

            Show
            jerome Jérôme Mouneyrac added a comment - no problem, it's done
            Hide
            nebgor 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
            nebgor 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
            nebgor 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
            nebgor 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
            jerome Jérôme Mouneyrac added a comment -

            I created a new branch, it should be clean

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

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

            Show
            samhemelryk Sam Hemelryk added a comment - Assigning to Eloy so that he can look at this on Wednesday
            Hide
            stronk7 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
            stronk7 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
            jerome 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
            jerome 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
            jerome Jérôme Mouneyrac added a comment -

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

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

            ok you can integrate MDL-30986-attempt2

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

            Stealing this off Eloy to help the merge pain

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

            Thanks Jerome, i've integrated this now

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

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

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

            This is great thanks a lot!

            Show
            jerome Jérôme Mouneyrac added a comment - This is great thanks a lot!
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  25/Jun/12