Moodle
  1. Moodle
  2. MDL-30987

message 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.3
    • Component/s: Documentation, Messages
    • Labels:
    • Rank:
      37391

      Description

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

        Raised MDL-31031 and MDL-31033 while doing this.

        Show
        Andrew Davis added a comment - Raised MDL-31031 and MDL-31033 while doing this.
        Hide
        Andrew Davis added a comment -

        This is still a work in progress. Should get this finished tomorrow (or maybe tonight).

        Show
        Andrew Davis added a comment - This is still a work in progress. Should get this finished tomorrow (or maybe tonight).
        Hide
        Andrew Davis added a comment -

        All done I think. The only thing Im not sure about is whether I have the package/subpackage information correct in the message processors. https://github.com/andyjdavis/moodle/compare/master...MDL-30987_message_docs#diff-19

        Putting this up for peer review to get some feedback.

        Show
        Andrew Davis added a comment - All done I think. The only thing Im not sure about is whether I have the package/subpackage information correct in the message processors. https://github.com/andyjdavis/moodle/compare/master...MDL-30987_message_docs#diff-19 Putting this up for peer review to get some feedback.
        Hide
        Andrew Davis added a comment -

        Ive put up a new version that I'm more confident is correct.

        Show
        Andrew Davis added a comment - Ive put up a new version that I'm more confident is correct.
        Hide
        David Mudrak added a comment -

        Hi Andrew, there is a missing var type in "@param $component"

        Show
        David Mudrak added a comment - Hi Andrew, there is a missing var type in "@param $component"
        Hide
        Rajesh Taneja added a comment -

        Hello Andrew,

        In addition to what David mentioned, you might want to consider following:

        1. message/externallib.php package should be core_message and subpackage should be removed. You might want plugin manager to look at this core_message_external and add category tag
        2. Prefix for Messaging consumers is message (/message/output) not sure if core_message_output_email is correct. IMO it should be core_message_email
        3. Please use datatype for $USER, $CFG as stdClass

        Rest looks Great, just wondering if category tag appropriately (as it's used at few places only.)

        Show
        Rajesh Taneja added a comment - Hello Andrew, In addition to what David mentioned, you might want to consider following: message/externallib.php package should be core_message and subpackage should be removed. You might want plugin manager to look at this core_message_external and add category tag Prefix for Messaging consumers is message (/message/output) not sure if core_message_output_email is correct. IMO it should be core_message_email Please use datatype for $USER, $CFG as stdClass Rest looks Great, just wondering if category tag appropriately (as it's used at few places only.)
        Hide
        Martin Dougiamas added a comment -

        The @category tag only needs to be on message_send(). That function is the main API for plugins to send messages, and so that is all that needs to be described (and in detail) on http://docs.moodle.org/dev/Message_API

        All the rest is for the message system itself.

        And the base class for message output processors is a different API, which is specific for that type of plugins and will be described on the Plugin overview page for that plugin type.

        Show
        Martin Dougiamas added a comment - The @category tag only needs to be on message_send(). That function is the main API for plugins to send messages, and so that is all that needs to be described (and in detail) on http://docs.moodle.org/dev/Message_API All the rest is for the message system itself. And the base class for message output processors is a different API, which is specific for that type of plugins and will be described on the Plugin overview page for that plugin type.
        Hide
        Andrew Davis added a comment -

        Ok, I think Ive got that stuff fixed. The only point I don't understand is "Please use datatype for $USER, $CFG as stdClass" Where are you talking about Rajesh?

        Show
        Andrew Davis added a comment - Ok, I think Ive got that stuff fixed. The only point I don't understand is "Please use datatype for $USER, $CFG as stdClass" Where are you talking about Rajesh?
        Hide
        Rajesh Taneja added a comment -

        In message/output/lib.php - Line: 66, I think it should be stdClass?
        Also, package is still core_message_output, shouldn't it be core_message, similarly core_message_output_popup should be core_message_popup.

        Show
        Rajesh Taneja added a comment - In message/output/lib.php - Line: 66, I think it should be stdClass? Also, package is still core_message_output, shouldn't it be core_message, similarly core_message_output_popup should be core_message_popup.
        Hide
        Andrew Davis added a comment -

        Ive fixed up that object/stdClass.

        Im pretty sure the packages are correct. The standard set of message processors we provide need to be grouped. Plus they all have class names like message_output_jabber and it seems like the package should match the class name structure.

        Show
        Andrew Davis added a comment - Ive fixed up that object/stdClass. Im pretty sure the packages are correct. The standard set of message processors we provide need to be grouped. Plus they all have class names like message_output_jabber and it seems like the package should match the class name structure.
        Hide
        Andrew Davis added a comment -

        After some discussion in a meeting I've updated the packages for the message processors to comply with http://docs.moodle.org/dev/Frankenstyle#Plugin_types

        I still think its a bad idea for the following reasons:
        1) The package names no longer conform to the same standard as the class names
        2) We are removing the ability to add a 2nd kind of plugin to the /message directory in the future.

        Show
        Andrew Davis added a comment - After some discussion in a meeting I've updated the packages for the message processors to comply with http://docs.moodle.org/dev/Frankenstyle#Plugin_types I still think its a bad idea for the following reasons: 1) The package names no longer conform to the same standard as the class names 2) We are removing the ability to add a 2nd kind of plugin to the /message directory in the future.
        Hide
        Andrew Davis added a comment -
        Show
        Andrew Davis added a comment - I've created http://docs.moodle.org/dev/Message_API
        Hide
        Sam Hemelryk added a comment -

        Hi Andrew, is there anything more than needs to be peer-reviewed in regards to the code here or can this be put up for integration?
        I had a quick look over the code and certainly the phpdocs are looking perfect.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Andrew, is there anything more than needs to be peer-reviewed in regards to the code here or can this be put up for integration? I had a quick look over the code and certainly the phpdocs are looking perfect. Cheers Sam
        Hide
        Andrew Davis added a comment -

        I think its ready to go. I was just waiting for someone to verify that. Putting it up for integration.

        Show
        Andrew Davis added a comment - I think its ready to go. I was just waiting for someone to verify that. Putting it up for integration.
        Hide
        Andrew Davis added a comment -

        Ive rebased against the latest code. Git did something weird during the rebase so I've pushed to MDL-30987_message_docs2 (instead of MDL-30987_message_docs) to preserve my old branch just in case something is broken.

        Show
        Andrew Davis added a comment - Ive rebased against the latest code. Git did something weird during the rebase so I've pushed to MDL-30987 _message_docs2 (instead of MDL-30987 _message_docs) to preserve my old branch just in case something is broken.
        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
        Tim Hunt added a comment -

        Sarcastic and unhelpful comments from me in Developer's chat:

        (3:07:27 PM) timhunt: Well, that is really helpful: https://github.com/andyjdavis/moodle/compare/master...MDL-30987_message_docs2#L21R25
        (3:07:27 PM) moodlebot: http://tracker.moodle.org/browse/MDL-30987 message API, check and update DocBlock - Assignee: Andrew Davis Type: Task Status: Waiting for integration review V:0 W:0 Priority: Minor
        (3:07:39 PM) timhunt: I hope most of the new API docs are better than that.
        (3:08:04 PM) timhunt: I mean wow! send_message processes messages, I could never have guessed that.
        (3:08:14 PM) timhunt: And it takes some data as an argument.
        (3:08:30 PM) timhunt: Those are all the details I need to know to use or override that method.

        Sorry, I am having a bad day. (Not helped by this.)

        Just thought you might want to know, so you can improve these docs.

        Show
        Tim Hunt added a comment - Sarcastic and unhelpful comments from me in Developer's chat: (3:07:27 PM) timhunt: Well, that is really helpful: https://github.com/andyjdavis/moodle/compare/master...MDL-30987_message_docs2#L21R25 (3:07:27 PM) moodlebot: http://tracker.moodle.org/browse/MDL-30987 message API, check and update DocBlock - Assignee: Andrew Davis Type: Task Status: Waiting for integration review V:0 W:0 Priority: Minor (3:07:39 PM) timhunt: I hope most of the new API docs are better than that. (3:08:04 PM) timhunt: I mean wow! send_message processes messages, I could never have guessed that. (3:08:14 PM) timhunt: And it takes some data as an argument. (3:08:30 PM) timhunt: Those are all the details I need to know to use or override that method. Sorry, I am having a bad day. (Not helped by this.) Just thought you might want to know, so you can improve these docs.
        Hide
        Andrew Davis added a comment -

        Ive added another commit that points users to where they can get details of what is in the arguments.

        Show
        Andrew Davis added a comment - Ive added another commit that points users to where they can get details of what is in the arguments.
        Hide
        Sam Hemelryk added a comment -

        Hi Andrew,

        I've just picked this up for integration, the first thing I have been doing before reviewing it myself it running Marina's moodlecheck plugin against changes to identify any obvious things.
        Several things came up when I ran it against the changed files most notable the things put into @package and functions with incomplete/incorrect @params.
        It's probably best you run the tool yourself and have a look at the output as it will give you a nice complete list.
        You can get the tool here https://github.com/marinaglancy/moodle-local_moodlecheck

        In regards to @package there has been quite a bit of confusion about this one (due to constantly changing/misunerstood rules and ideas).
        @package should be a valid component so for the messaging API core_message.
        For messaging output plugins it should be the frankenstyle name for the plugin so for the popup plugin I think it would be message_popup.

        Let me know if you have any further questions.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Andrew, I've just picked this up for integration, the first thing I have been doing before reviewing it myself it running Marina's moodlecheck plugin against changes to identify any obvious things. Several things came up when I ran it against the changed files most notable the things put into @package and functions with incomplete/incorrect @params. It's probably best you run the tool yourself and have a look at the output as it will give you a nice complete list. You can get the tool here https://github.com/marinaglancy/moodle-local_moodlecheck In regards to @package there has been quite a bit of confusion about this one (due to constantly changing/misunerstood rules and ideas). @package should be a valid component so for the messaging API core_message. For messaging output plugins it should be the frankenstyle name for the plugin so for the popup plugin I think it would be message_popup. Let me know if you have any further questions. Cheers Sam
        Hide
        Andrew Davis added a comment -

        Thats weird. I was getting a clean bill of health from the code checker. Getting the latest version now.

        Show
        Andrew Davis added a comment - Thats weird. I was getting a clean bill of health from the code checker. Getting the latest version now.
        Hide
        Andrew Davis added a comment -

        I've made a bunch more changes and squished the commits. All messaging code is now passing the code checker whether it is API related or not.

        The code checker has changed a fair bit. I probably should have been keeping it up to date.

        Show
        Andrew Davis added a comment - I've made a bunch more changes and squished the commits. All messaging code is now passing the code checker whether it is API related or not. The code checker has changed a fair bit. I probably should have been keeping it up to date.
        Hide
        Sam Hemelryk added a comment -

        Thanks Andrew, this has been integrated now. Just noting I did make an additional commit just to fix up a couple of very minor things as well.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now. Just noting I did make an additional commit just to fix up a couple of very minor things as well. Cheers Sam
        Hide
        Michael de Raadt added a comment -
        Show
        Michael de Raadt added a comment - Reading the docs at http://docs.moodle.org/dev/Message_API
        Hide
        Michael de Raadt added a comment -

        Test result Great.

        I made a few minor edits, but the documentation was very good.

        Show
        Michael de Raadt added a comment - Test result Great. I made a few minor edits, but the documentation was very good.
        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: