Moodle
  1. Moodle
  2. MDL-30988

preference API, check and update DocBlock

    Details

    • Rank:
      37392

      Description

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

          Rajesh Taneja created issue -
          Rajesh Taneja made changes -
          Field Original Value New Value
          Link This issue has a clone MDL-30987 [ MDL-30987 ]
          Rajesh Taneja made changes -
          Link This issue has a clone MDL-30987 [ MDL-30987 ]
          Rajesh Taneja made changes -
          Description Check and update documentation, so that it should comply with [moodle coding guidelines|http://docs.moodle.org/dev/Coding_style#Files].
          Following needs to be updated/checked for message api
          # DocBlock for page and functions.
          # All the files should be checked/updated.

          Note: You can create sub-tasks, so as to avoid bulk integration.
          Check and update documentation, so that it should comply with [moodle coding guidelines|http://docs.moodle.org/dev/Coding_style#Files].
          Following needs to be updated/checked for preference api
          # DocBlock for page and functions.
          # All the files should be checked/updated.

          Note: You can create sub-tasks, so as to avoid bulk integration.
          Component/s Administration [ 10050 ]
          Component/s Messages [ 10084 ]
          Rajesh Taneja made changes -
          Link This issue is a clone of MDL-30989 [ MDL-30989 ]
          Rajesh Taneja made changes -
          Link This issue is a clone of MDL-30989 [ MDL-30989 ]
          Rajesh Taneja made changes -
          Component/s Documentation [ 10073 ]
          Rajesh Taneja made changes -
          Assignee Rajesh Taneja [ rajeshtaneja ] moodle.com [ moodle.com ]
          Michael de Raadt made changes -
          Affects Version/s STABLE backlog [ 10463 ]
          Affects Version/s 2.2 [ 10656 ]
          Affects Version/s 2.3 [ 10657 ]
          Labels triaged
          Fix Version/s 2.2.1 [ 11456 ]
          Fix Version/s 2.2 [ 10656 ]
          Michael de Raadt made changes -
          Assignee moodle.com [ moodle.com ] Jerome Mouneyrac [ jerome ]
          Michael de Raadt made changes -
          Assignee Jerome Mouneyrac [ jerome ] Gerard Caulfield [ gerry ]
          Michael de Raadt made changes -
          Affects Version/s Docs Sprint [ 11551 ]
          Affects Version/s STABLE backlog [ 10463 ]
          Michael de Raadt made changes -
          Affects Version/s 2.2 [ 10656 ]
          Affects Version/s 2.2.1 [ 11456 ]
          Affects Version/s Docs Sprint [ 11551 ]
          Fix Version/s Docs Sprint [ 11551 ]
          Fix Version/s 2.2 [ 10656 ]
          Fix Version/s 2.2.1 [ 11456 ]
          Gerard Caulfield made changes -
          Status Open [ 1 ] Development in progress [ 3 ]
          Gerard Caulfield made changes -
          Pull Master Diff URL https://github.com/gerrywastaken/moodle/compare/master...m_MDL-30988_adding_preference_api_phpdocs
          Pull Master Branch m_MDL-30988_adding_preference_api_phpdocs
          Pull 2.2 Diff URL https://github.com/gerrywastaken/moodle/compare/MOODLE_22_STABLE...22_MDL-30988_adding_preference_api_phpdocs
          Pull 2.2 Branch 22_MDL-30988_adding_preference_api_phpdocs
          Pull from Repository git@github.com:gerrywastaken/moodle.git
          Gerard Caulfield made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          Gerard Caulfield made changes -
          Status Waiting for peer review [ 10012 ] Development in progress [ 3 ]
          Gerard Caulfield made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          Hide
          David Mudrak added a comment -

          Using "unset" in this context does not look valid, we use null (the implicit value) for these cases

          @param stdClass|int|unset $user

          Show
          David Mudrak added a comment - Using "unset" in this context does not look valid, we use null (the implicit value) for these cases @param stdClass|int|unset $user
          Hide
          Gerard Caulfield added a comment -

          We decided on using the short versions of the types given here: http://php.net/manual/en/language.types.type-juggling.php

          Shown under "Type Casting".

          Show
          Gerard Caulfield added a comment - We decided on using the short versions of the types given here: http://php.net/manual/en/language.types.type-juggling.php Shown under "Type Casting".
          Gerard Caulfield made changes -
          Status Waiting for peer review [ 10012 ] Development in progress [ 3 ]
          Hide
          Gerard Caulfield added a comment -

          I agree with you though, so I'll change it.

          Show
          Gerard Caulfield added a comment - I agree with you though, so I'll change it.
          Hide
          Gerard Caulfield added a comment - - edited

          Changed the types suggested by David. I also set them to mixed to be consistent as everybody else appears to be doing that.

          Show
          Gerard Caulfield added a comment - - edited Changed the types suggested by David. I also set them to mixed to be consistent as everybody else appears to be doing that.
          Gerard Caulfield made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          Gerard Caulfield made changes -
          Status Waiting for peer review [ 10012 ] Development in progress [ 3 ]
          Hide
          Gerard Caulfield added a comment -

          Updating again after new info surfaced in scrum about how to best describe variable types. Previously I was told it would be best to use mixed when there are multiple types, but in scrum it came out that it would be better to list all types that are accepted and returned (which is what I was originally doing).

          So now I've fixed it, while keeping David's suggestions.

          Show
          Gerard Caulfield added a comment - Updating again after new info surfaced in scrum about how to best describe variable types. Previously I was told it would be best to use mixed when there are multiple types, but in scrum it came out that it would be better to list all types that are accepted and returned (which is what I was originally doing). So now I've fixed it, while keeping David's suggestions.
          Gerard Caulfield made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          Hide
          Rajesh Taneja added a comment -

          Hello Gerry,

          You might want to add package tag in function dockblock, as page docblock has @package core.
          Rest looks Good.

          Show
          Rajesh Taneja added a comment - Hello Gerry, You might want to add package tag in function dockblock, as page docblock has @package core. Rest looks Good.
          Gerard Caulfield made changes -
          Status Waiting for peer review [ 10012 ] Development in progress [ 3 ]
          Gerard Caulfield made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          Hide
          Gerard Caulfield added a comment -

          Thanks Rajesh. I've added
          @package core_preference

          Show
          Gerard Caulfield added a comment - Thanks Rajesh. I've added @package core_preference
          Gerard Caulfield made changes -
          Status Waiting for peer review [ 10012 ] Development in progress [ 3 ]
          Hide
          Gerard Caulfield added a comment -

          I need to re-add "@return void" in places where it existed as a result of Jan 10th 3pm dev chat discussion

          Show
          Gerard Caulfield added a comment - I need to re-add "@return void" in places where it existed as a result of Jan 10th 3pm dev chat discussion
          Rajesh Taneja made changes -
          Parent MDL-30971 [ 50000 ]
          Issue Type Sub-task [ 5 ] Task [ 3 ]
          Hide
          Martin Dougiamas added a comment -

          The @category preference tags all look to be on the right functions here, great!

          Can you summarise the use of those on http://docs.moodle.org/dev/Preference_API ? Cheers

          Show
          Martin Dougiamas added a comment - The @category preference tags all look to be on the right functions here, great! Can you summarise the use of those on http://docs.moodle.org/dev/Preference_API ? Cheers
          Gerard Caulfield made changes -
          Hide
          Gerard Caulfield added a comment -

          Adding main ticket link

          Show
          Gerard Caulfield added a comment - Adding main ticket link
          Gerard Caulfield made changes -
          Link This issue will help resolve MDL-30971 [ MDL-30971 ]
          Hide
          Gerard Caulfield added a comment -

          I'm still working on the wiki page. But it should be done soon.

          Show
          Gerard Caulfield added a comment - I'm still working on the wiki page. But it should be done soon.
          Gerard Caulfield made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          Hide
          Gerard Caulfield added a comment -

          The preference Doc is now in a state where I believe it's pretty friendly for beginner through to advanced plug-in developers: http://docs.moodle.org/dev/Preference_API

          I tried to say as much as I could in as few words as possible, while still staying readable. But let me know if you have any suggestions that do not unnecessarily make the document more complicated.

          Show
          Gerard Caulfield added a comment - The preference Doc is now in a state where I believe it's pretty friendly for beginner through to advanced plug-in developers: http://docs.moodle.org/dev/Preference_API I tried to say as much as I could in as few words as possible, while still staying readable. But let me know if you have any suggestions that do not unnecessarily make the document more complicated.
          Sam Hemelryk made changes -
          Original Estimate 0 minutes [ 0 ]
          Remaining Estimate 0 minutes [ 0 ]
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          Peer reviewer samhemelryk
          Hide
          Sam Hemelryk added a comment -

          Hi Gerry,

          Changes to code look really good thanks.
          I've read through the doc and I have a couple of ideas about how that could be improved.

          1. Move the database structure down to the bottom (but still above see also). The database structure is interesting but most people arriving to this API would like want to read the overview and usage examples before analysing the database structure. (perhaps also add the table name somewhere)
          2. There's an error with the database structure in that the value field length changed between versions. This was done because developers are using these funcs more and more and are trying to store increasingly larger data/structures in the there.
          3. It may be worthwhile including some text to describe when to use the user preference API and include reference to examples of what it is being used for presently or something. Really just illustrating that anything relating to a single user can be stored there, normally state indicators are stored there and it can be as simple as a yes no, or as complex as a full blown structure (check out how gradebook uses it).
          4. It would also be worth including concepts we have for user preferences. For instance if you have a user preference that is true or false you should never actually set it to false, you should set the default to false and if it is to be set to false then unset it instead. Actually perhaps thats the only concept!
          5. Maybe worth mentioning the JS implementation of this API as well. I don't think we are meant to be documenting them (although it would be nice to include reference in the docs).

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Gerry, Changes to code look really good thanks. I've read through the doc and I have a couple of ideas about how that could be improved. Move the database structure down to the bottom (but still above see also). The database structure is interesting but most people arriving to this API would like want to read the overview and usage examples before analysing the database structure. (perhaps also add the table name somewhere) There's an error with the database structure in that the value field length changed between versions. This was done because developers are using these funcs more and more and are trying to store increasingly larger data/structures in the there. It may be worthwhile including some text to describe when to use the user preference API and include reference to examples of what it is being used for presently or something. Really just illustrating that anything relating to a single user can be stored there, normally state indicators are stored there and it can be as simple as a yes no, or as complex as a full blown structure (check out how gradebook uses it). It would also be worth including concepts we have for user preferences. For instance if you have a user preference that is true or false you should never actually set it to false, you should set the default to false and if it is to be set to false then unset it instead. Actually perhaps thats the only concept! Maybe worth mentioning the JS implementation of this API as well. I don't think we are meant to be documenting them (although it would be nice to include reference in the docs). Cheers Sam
          Sam Hemelryk made changes -
          Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
          Hide
          Sam Hemelryk added a comment -

          LOL thinking on point 5, check out lib/ajax/setuserpref.php we should probably fix the @package there as part of this issue.

          Show
          Sam Hemelryk added a comment - LOL thinking on point 5, check out lib/ajax/setuserpref.php we should probably fix the @package there as part of this issue.
          Hide
          Sam Hemelryk added a comment -

          LOL thinking on point 5, check out lib/ajax/setuserpref.php we should probably fix the @package there as part of this issue.

          Show
          Sam Hemelryk added a comment - LOL thinking on point 5, check out lib/ajax/setuserpref.php we should probably fix the @package there as part of this issue.
          Gerard Caulfield made changes -
          Hide
          Gerard Caulfield added a comment -

          @Sam re comment-139502

          1. I agree, I wanted to put the database structure at the end. However, I used and I saw somebody else get blasted for not following the format of earlier docs and so this made me change my mind. However I'll gladly make this change.
          2. Sure I'll look into that.
          3. Martin had earlier made the point that our docs were too wordy and we should keep our new docs to the essentials that people needed to know and that they could go to the generated phpdocs or the code if they wanted to do something advanced.
          4. I wasn't aware of that, I'll make the change.
          5. Sure thing.
          Show
          Gerard Caulfield added a comment - @Sam re comment-139502 I agree, I wanted to put the database structure at the end. However, I used and I saw somebody else get blasted for not following the format of earlier docs and so this made me change my mind. However I'll gladly make this change. Sure I'll look into that. Martin had earlier made the point that our docs were too wordy and we should keep our new docs to the essentials that people needed to know and that they could go to the generated phpdocs or the code if they wanted to do something advanced. I wasn't aware of that, I'll make the change. Sure thing.
          Hide
          Gerard Caulfield added a comment -

          Actually upon further thought, I agree with point #3, I'll add pretty much exactly what you said.

          Show
          Gerard Caulfield added a comment - Actually upon further thought, I agree with point #3, I'll add pretty much exactly what you said.
          Gerard Caulfield made changes -
          Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
          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
          Gerard Caulfield added a comment -

          Rebased

          Show
          Gerard Caulfield added a comment - Rebased
          Aparup Banerjee made changes -
          Currently in integration Yes [ 10041 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Bad repository url

          Show
          Eloy Lafuente (stronk7) added a comment - Bad repository url
          Hide
          Gerard Caulfield added a comment -

          Ooops, thanks, fixed the url

          Show
          Gerard Caulfield added a comment - Ooops, thanks, fixed the url
          Gerard Caulfield made changes -
          Pull from Repository git@github.com:gerrywastaken/moodle.git git://github.com/gerrywastaken/moodle.git
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Gerard,

          everything looks perfect from a phpdocs POV, but:

          1) there are 2-3 occurrences of "@package core_preference" and that's not a valid package at all. "preference" is a correct API but not a package (is not any valid component name). Should be "core" instead" if I'm not wrong.

          2) The lines:

          ///////////////////////////////////////////////////////
          //////////////// USER PREFERENCE API //////////////////
          

          although nothing has been determined about them, they really look aged (plus are detected as 3-slashes invalid comments). Perhaps a simpler "// USER PREFERENCE API" is enough... for your consideration.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Gerard, everything looks perfect from a phpdocs POV, but: 1) there are 2-3 occurrences of "@package core_preference" and that's not a valid package at all. "preference" is a correct API but not a package (is not any valid component name). Should be "core" instead" if I'm not wrong. 2) The lines: /////////////////////////////////////////////////////// //////////////// USER PREFERENCE API ////////////////// although nothing has been determined about them, they really look aged (plus are detected as 3-slashes invalid comments). Perhaps a simpler "// USER PREFERENCE API" is enough... for your consideration. Ciao
          Hide
          Gerard Caulfield added a comment -

          Ah... well I did ask others before picking core_preference, but what you say makes sense.

          For the second point I wanted to make it consistent with the rest of the file, however if others making changes to lib/moodlelib.php are being asked to make the same change to their sections, then it makes sense.

          Both changes made, thanks for the feedback.

          Show
          Gerard Caulfield added a comment - Ah... well I did ask others before picking core_preference, but what you say makes sense. For the second point I wanted to make it consistent with the rest of the file, however if others making changes to lib/moodlelib.php are being asked to make the same change to their sections, then it makes sense. Both changes made, thanks for the feedback.
          Sam Hemelryk made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator samhemelryk
          Hide
          Sam Hemelryk added a comment -

          Thanks Gerry this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Gerry this has been integrated now
          Sam Hemelryk made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Fix Version/s 2.3 [ 10657 ]
          Michael de Raadt made changes -
          Tester abgreeve
          Adrian Greeve made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Hide
          Adrian Greeve added a comment -

          I had a look at the master diff. ajaxlib.php and setuserpref.php @package need to be changed to core.
          Everything else looks fine.

          Show
          Adrian Greeve added a comment - I had a look at the master diff. ajaxlib.php and setuserpref.php @package need to be changed to core. Everything else looks fine.
          Adrian Greeve made changes -
          Status Testing in progress [ 10011 ] Problem during testing [ 10007 ]
          Hide
          Gerard Caulfield added a comment -

          I must have missed this, however it seems Sam noticed during integration and fixed it. I've updated the patch to reflect this change, but integration already has it thanks to Sam.

          Show
          Gerard Caulfield added a comment - I must have missed this, however it seems Sam noticed during integration and fixed it. I've updated the patch to reflect this change, but integration already has it thanks to Sam.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          So... this can be safely passed, correct?

          Show
          Eloy Lafuente (stronk7) added a comment - So... this can be safely passed, correct?
          Hide
          Adrian Greeve added a comment -

          Yes it can. Sorry about the delay. Though I did notice that in lib/ajaxlib.php at line 21 we have - @package moodlecore I'm not sure if this is valid or not. If it is then please integrate.

          Show
          Adrian Greeve added a comment - Yes it can. Sorry about the delay. Though I did notice that in lib/ajaxlib.php at line 21 we have - @package moodlecore I'm not sure if this is valid or not. If it is then please integrate.
          Sam Hemelryk made changes -
          Status Problem during testing [ 10007 ] Integration review in progress [ 10004 ]
          Sam Hemelryk made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Sam Hemelryk made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Hide
          Sam Hemelryk added a comment -

          Thanks Adrian, I've marked this passed now.
          I've also made an additional commit to fix up the ajaxlib package.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Adrian, I've marked this passed now. I've also made an additional commit to fix up the ajaxlib package. Cheers Sam
          Sam Hemelryk made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          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
          Eloy Lafuente (stronk7) made changes -
          Status Tested [ 10006 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Currently in integration Yes [ 10041 ]
          Integration date 23/Feb/12
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s Docs Sprint [ 11551 ]

            People

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

              Dates

              • Created:
                Updated:
                Resolved: