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

          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".
          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.
          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.
          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.
          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
          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
          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
          Hide
          Gerard Caulfield added a comment -

          Adding main ticket link

          Show
          Gerard Caulfield added a comment - Adding main ticket link
          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.
          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.
          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
          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.
          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.
          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
          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
          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.
          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
          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.
          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.
          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
          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:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: