Details

    • Rank:
      37376

      Description

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

          the crappiest part is that i cannot create sub-tasks!

          Show
          Aparup Banerjee added a comment - the crappiest part is that i cannot create sub-tasks!
          Hide
          Aparup Banerjee added a comment -

          linking to meta

          Show
          Aparup Banerjee added a comment - linking to meta
          Hide
          Aparup Banerjee added a comment -

          alright i've put up a branch for dml API's phpdoc changes.

          Show
          Aparup Banerjee added a comment - alright i've put up a branch for dml API's phpdoc changes.
          Hide
          Sam Hemelryk added a comment -

          Hi dude,
          Just been looking over this now and have made the following notes:

          In general:

          1. Param types seperated by |'s should have spaces. Pretty sure phpdoc explodes by space when processing those (limited to 3 bits)
          1. database_column_info.php
            • Alignment of @category on the files docblock
            • Has an @subpackage database which is inconsistent
            • @param on __construct has the args round the wrong way
          2. moodle_database.php
            • Check alignment in the classes docblock
            • $dbpass on connnect method comment could be made clearer.
            • Also on store_settings, create_database, method
            • fix_table_names @param is missing $ for $sql
            • _fix_sql_params_dollar_callback Needs a proper docblock
            • get_tables missing @param
            • get_indexes missing @param
            • get_recordset $limitfrom $limitnum do not require both to be set (referenced in the comments for those args)
            • Same as above in other functions with those args
            • get_records_list wrong @param type for values array
            • get_record @todo for strictness needs to be moved to a new line, @todo's arn't an inline thing. Should also add a comment about what the todo is about.
            • insert_record_raw typo on param @returnit
            • update_record_raw @param for bulk missing $bulk
            • Same as above for update_record and update_record_raw
            • sql_like typo in @param for accentsensitive. (perhaps this function could have a more meaningful description)
            • sql_ilike should have a deprecated since version and perhaps a new issue to clean up deprecated methods in database if not already exists
            • sql_concat There is an @param method of specifying 1..n arguments which could be used here. From the manual '@param mixed $v,... unlimited OPTIONAL number of additional variables'
          3. moodle_recordset.php
            • moodle_recordset missing required phpdoc stuff, package, license etc
          4. moodle_templates.php
            • moodle_tempaltes class missing phpdoc block altogether
            • properties need docblocks
            • __construct @param is borked
          5. moodle_transactions.php
            • moodle_transaction class needs @license and friends
            • properties need docblocks
            • lol to return of dispose, should be null
          6. sqlsrv_native_moodle_temptables class needs license et al.
          7. sqlsrv_native_moodle_recordset needs 100% review

          Hmm stopping there dude seems that most of the db engine files need review again.
          Some general notes:

          • license, copyright, and package on classes.
          • properties need docblocks
          • methods need docblocks

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi dude, Just been looking over this now and have made the following notes: In general: Param types seperated by |'s should have spaces. Pretty sure phpdoc explodes by space when processing those (limited to 3 bits) database_column_info.php Alignment of @category on the files docblock Has an @subpackage database which is inconsistent @param on __construct has the args round the wrong way moodle_database.php Check alignment in the classes docblock $dbpass on connnect method comment could be made clearer. Also on store_settings, create_database, method fix_table_names @param is missing $ for $sql _fix_sql_params_dollar_callback Needs a proper docblock get_tables missing @param get_indexes missing @param get_recordset $limitfrom $limitnum do not require both to be set (referenced in the comments for those args) Same as above in other functions with those args get_records_list wrong @param type for values array get_record @todo for strictness needs to be moved to a new line, @todo's arn't an inline thing. Should also add a comment about what the todo is about. insert_record_raw typo on param @returnit update_record_raw @param for bulk missing $bulk Same as above for update_record and update_record_raw sql_like typo in @param for accentsensitive. (perhaps this function could have a more meaningful description) sql_ilike should have a deprecated since version and perhaps a new issue to clean up deprecated methods in database if not already exists sql_concat There is an @param method of specifying 1..n arguments which could be used here. From the manual '@param mixed $v,... unlimited OPTIONAL number of additional variables' moodle_recordset.php moodle_recordset missing required phpdoc stuff, package, license etc moodle_templates.php moodle_tempaltes class missing phpdoc block altogether properties need docblocks __construct @param is borked moodle_transactions.php moodle_transaction class needs @license and friends properties need docblocks lol to return of dispose, should be null sqlsrv_native_moodle_temptables class needs license et al. sqlsrv_native_moodle_recordset needs 100% review Hmm stopping there dude seems that most of the db engine files need review again. Some general notes: license, copyright, and package on classes. properties need docblocks methods need docblocks Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          Thanks Sam,

          i wasn't sure if non API (ie: db engines) was needing phpdoc updates right now but i'm sure i'll get around to that..

          • accentsensitive is still not too clear to me - its some sort of european language thing?
          Show
          Aparup Banerjee added a comment - Thanks Sam, i wasn't sure if non API (ie: db engines) was needing phpdoc updates right now but i'm sure i'll get around to that.. accentsensitive is still not too clear to me - its some sort of european language thing?
          Hide
          Aparup Banerjee added a comment -

          ok i've updated with changes almost all of Sam's review comments.. for the others, notes as follows:

          • 4.1 its the only class , hence going with the class docblock is the file's docblock > http://docs.moodle.org/dev/Coding_style#Classes_3
          • 5.1 same as above
          • 6, 7,...dbengines : i've made more change to db drivers. particularly, noting that :
            • i've added '@package core_dml' to engine driver specific files so that they don't show up under 'core' API. They are rather a sub-system even though they aren't listed under get_core_subsystems().
            • with above change, there are now two packages 'core' and 'core_dml' so i've used '@subpackage dml' to link them all up.
            • '@category dml' is only used in moodle_*.php files which are core API and which we want to highlight.
            • specifically got SamH's +1 for '@package core_dml' hehe

          all changes up on github now.

          Show
          Aparup Banerjee added a comment - ok i've updated with changes almost all of Sam's review comments.. for the others, notes as follows: 4.1 its the only class , hence going with the class docblock is the file's docblock > http://docs.moodle.org/dev/Coding_style#Classes_3 5.1 same as above 6, 7,...dbengines : i've made more change to db drivers. particularly, noting that : i've added '@package core_dml' to engine driver specific files so that they don't show up under 'core' API. They are rather a sub-system even though they aren't listed under get_core_subsystems(). with above change, there are now two packages 'core' and 'core_dml' so i've used '@subpackage dml' to link them all up. '@category dml' is only used in moodle_*.php files which are core API and which we want to highlight. specifically got SamH's +1 for '@package core_dml' hehe all changes up on github now.
          Hide
          Andrew Davis added a comment - - edited

          Your branch needs to be rebased or something. Its showing ~100 commits by lots of people.

          Show
          Andrew Davis added a comment - - edited Your branch needs to be rebased or something. Its showing ~100 commits by lots of people.
          Hide
          Aparup Banerjee added a comment - - edited

          Thanks, i've updated my copy of master on github now...

          also just added commit for lib/dmllib.php which i missed out on (outside lib/dml)

          • i think i forgot to add i didn't do much of phpdocs on the db drivers as they aren't really API.
          Show
          Aparup Banerjee added a comment - - edited Thanks, i've updated my copy of master on github now... also just added commit for lib/dmllib.php which i missed out on (outside lib/dml) i think i forgot to add i didn't do much of phpdocs on the db drivers as they aren't really API.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, after chatting a bit with Apu...

          1) I don't buy the idea of introducing "invented" packages. Packages can only be valid component names. So, if we are missing core_dml or core_database or so, first we need to propose its creation and, once created, start using it. Until then, I'd be using the "best" one available, that seems to be "core".

          2a) In fact, once everything gets handled by System X, we'll know if we need to create those new subsystems or no. But we won't know that before seeing the results. So surely "core" is the best for the time being.

          2b) Or alternatively, feel free to ignite the discussion about new subsystems being necessary since now @ HQ / wherever. And if it gets done soon, then you will be able to use it.

          3) But avoid using invented ones, in fact they will be failing CI tests 99% sure. Only valid component names, plz.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, after chatting a bit with Apu... 1) I don't buy the idea of introducing "invented" packages. Packages can only be valid component names. So, if we are missing core_dml or core_database or so, first we need to propose its creation and, once created, start using it. Until then, I'd be using the "best" one available, that seems to be "core". 2a) In fact, once everything gets handled by System X, we'll know if we need to create those new subsystems or no. But we won't know that before seeing the results. So surely "core" is the best for the time being. 2b) Or alternatively, feel free to ignite the discussion about new subsystems being necessary since now @ HQ / wherever. And if it gets done soon, then you will be able to use it. 3) But avoid using invented ones, in fact they will be failing CI tests 99% sure. Only valid component names, plz. Ciao
          Hide
          Aparup Banerjee added a comment -

          peer reviewer / integrator(yes, you): in response to Eloy's (2a) i've created

          branch : MDL-30972_2aha
          diff url: https://github.com/nebgor/moodle/compare/mistress...MDL-30972_2aha

          which goes along the lines of (2a) but also uses @subpackage to distinguish drivers seemingly harmlessly

          Show
          Aparup Banerjee added a comment - peer reviewer / integrator(yes, you): in response to Eloy's (2a) i've created branch : MDL-30972 _2aha diff url: https://github.com/nebgor/moodle/compare/mistress...MDL-30972_2aha which goes along the lines of (2a) but also uses @subpackage to distinguish drivers seemingly harmlessly
          Hide
          Aparup Banerjee added a comment -

          pushing on for integration
          (worried it'll get lost from radar, so onto Integrator's trusty radar.)

          Show
          Aparup Banerjee added a comment - pushing on for integration (worried it'll get lost from radar, so onto Integrator's trusty radar.)
          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
          Eloy Lafuente (stronk7) added a comment - - edited

          Apu, take a look to the results of the execution of the "Precheck remote branch" job in my jenkins server, I've executed it both for your original and the "2aha" branches. And ignoring the line length results (smurf.xml)... it seems that there are a bunch of methods missing the phpdocs for some params...

          Ciao

          PS: I won't be online till EU evening... feel free to launch any pre-check for any issue for your convenience... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Apu, take a look to the results of the execution of the "Precheck remote branch" job in my jenkins server, I've executed it both for your original and the "2aha" branches. And ignoring the line length results (smurf.xml)... it seems that there are a bunch of methods missing the phpdocs for some params... Ciao PS: I won't be online till EU evening... feel free to launch any pre-check for any issue for your convenience... ciao
          Hide
          Aparup Banerjee added a comment -

          thanks Eloy,
          i've made those changes, i ran the 'precheck' but soon realised i made the changes on the wrong branch.. hence i've overwritten my '2aha' branch , i'm trying to play with git fsck to recover the commit, failing that i'll redo my 'core_dml -> core' changes.

          Show
          Aparup Banerjee added a comment - thanks Eloy, i've made those changes, i ran the 'precheck' but soon realised i made the changes on the wrong branch.. hence i've overwritten my '2aha' branch , i'm trying to play with git fsck to recover the commit, failing that i'll redo my 'core_dml -> core' changes.
          Hide
          Aparup Banerjee added a comment -

          ok i've updated git repo and branches to point to MDL-30972_final (i finally found my modified branch at home, not @ HQ)

          the changes should be in there now.

          Show
          Aparup Banerjee added a comment - ok i've updated git repo and branches to point to MDL-30972 _final (i finally found my modified branch at home, not @ HQ) the changes should be in there now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yay, near, near... it seems that there are yet a bunch of incorrect @see inline tags.

          See http://stronk7.doesntexist.com/job/Precheck%20remote%20branch/90/artifact/work/smurf.xml

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yay, near, near... it seems that there are yet a bunch of incorrect @see inline tags. See http://stronk7.doesntexist.com/job/Precheck%20remote%20branch/90/artifact/work/smurf.xml Ciao
          Hide
          Aparup Banerjee added a comment -
          Show
          Aparup Banerjee added a comment - tons of old inline @see have been put onto new lines. smurfs up @ http://stronk7.doesntexist.com/view/prehecker/job/Precheck%20remote%20branch/103/artifact/work/smurf.xml
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Oh, I had supposed that you were going to convert them to proper

          {@link ...}

          inline tags! In fact phpdocs like:

          ...
          ...
               * Only records where $field takes one of the values $values are returned.
               * $values must be an array of values.
               *
               * Other arguments and the return type are like:
               * @see function get_recordset.
               *
               * @param string $table the table to query.
               * @param string $field a field to check (optional).
          ...
          ...
          

          Have no sense at all, the "Other arguments and the return type are like:" line will be considered "description" and will be put on top of all the tags. In other words, phpdoc blocks have 2 clear and separated parts:

          1) the description part, without tags (but the inline

          {@link xxx}

          one).
          2) the tags part, all the lines starting by @

          I think that the example above should be something like:

          ...
          ...
               * Only records where $field takes one of the values $values are returned.
               * $values must be an array of values.
               *
               * Other arguments and the return type are like the function {@link get_recordset()}.
               *
               * @param string $table the table to query.
               * @param string $field a field to check (optional).
          ...
          ...
          

          I've added one quick screenshot (using phpdocumentor) to show how it looks by default

          Ciao

          PS: Sorry for the multiple edits to the comment, my return key was clearly abused.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Oh, I had supposed that you were going to convert them to proper {@link ...} inline tags! In fact phpdocs like: ... ... * Only records where $field takes one of the values $values are returned. * $values must be an array of values. * * Other arguments and the return type are like: * @see function get_recordset. * * @param string $table the table to query. * @param string $field a field to check (optional). ... ... Have no sense at all, the "Other arguments and the return type are like:" line will be considered "description" and will be put on top of all the tags. In other words, phpdoc blocks have 2 clear and separated parts: 1) the description part, without tags (but the inline {@link xxx} one). 2) the tags part, all the lines starting by @ I think that the example above should be something like: ... ... * Only records where $field takes one of the values $values are returned. * $values must be an array of values. * * Other arguments and the return type are like the function {@link get_recordset()}. * * @param string $table the table to query. * @param string $field a field to check (optional). ... ... I've added one quick screenshot (using phpdocumentor) to show how it looks by default Ciao PS: Sorry for the multiple edits to the comment, my return key was clearly abused.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Attached one more screenshot, showing the same docs but with the correct

          {@link}

          inline tag in action.

          Show
          Eloy Lafuente (stronk7) added a comment - Attached one more screenshot, showing the same docs but with the correct {@link} inline tag in action.
          Hide
          Aparup Banerjee added a comment -

          thanks Eloy, those screen-shots really show what system X would handle them as much clearer.

          I've updated the branch to use @link more now.

          Show
          Aparup Banerjee added a comment - thanks Eloy, those screen-shots really show what system X would handle them as much clearer. I've updated the branch to use @link more now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Nobody tested this. Passing.

          Show
          Eloy Lafuente (stronk7) added a comment - Nobody tested this. Passing.
          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: