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:

      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.

        Gliffy Diagrams

          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: