Moodle
  1. Moodle
  2. MDL-30992

tag API, check and update DocBlock

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Documentation, Tags
    • Labels:
    • Rank:
      37396

      Description

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

      1. smurf.xml
        6 kB
        Aparup Banerjee

        Issue Links

          Activity

          Hide
          Gerard Caulfield added a comment -

          Adding main ticket link

          Show
          Gerard Caulfield added a comment - Adding main ticket link
          Hide
          Michael de Raadt added a comment - - edited

          Some comments:

          tag/lib.php

          • Lines 52-62, defines need comments (see style guide)
          • Lines 77, 147, 168, closing parentheses needed
          • Line 262, could use @see; and when should this function be called?
          • Line 455, "which are a calculated" should be "which are calculated" (unless your Italian)
          • Line 738, colon should be semi-colon (now that's pedantic)
          • Line 1104, unnecessary comma

          Very clean and consistent. My comments were really minor compared to other APIs I've reviewed.

          After considering these minor changes, push this to integration.

          Show
          Michael de Raadt added a comment - - edited Some comments: tag/lib.php Lines 52-62, defines need comments (see style guide) Lines 77, 147, 168, closing parentheses needed Line 262, could use @see; and when should this function be called? Line 455, "which are a calculated" should be "which are calculated" (unless your Italian) Line 738, colon should be semi-colon (now that's pedantic) Line 1104, unnecessary comma Very clean and consistent. My comments were really minor compared to other APIs I've reviewed. After considering these minor changes, push this to integration.
          Hide
          Gerard Caulfield added a comment -

          Thanks for all that Michael. Well spotted on those errors and I completely agreed with your change for line 738 once it was pointed out. All have been fixed.

          Show
          Gerard Caulfield added a comment - Thanks for all that Michael. Well spotted on those errors and I completely agreed with your change for line 738 once it was pointed out. All have been fixed.
          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
          Aparup Banerjee added a comment -

          Hi Gerry,
          I've attached a smurf.xml here to the issue.

          Please have a look through it:

          • formatting : we need to remove the trailing whitespaces. I always use 'git diff' and it shows them to me in red.
          • phpdoc : theres a few functions in there that have incomplete.
          • coursetagslib.php line 34 : param description could use a description of the variable, suggest something like: "A course id. 0 means all courses." We need to bear in mind that this might be the first function a developer might be trying to use.
          • coursetagslib.php line 36 : Also i'm thinking specifying $tagtype types there would easily be obsolete as a description once types change, i would suggest 'The type of tag, empty string returns all types' or perhaps a @see is better here? The function doesn't limit the range of types used.
          • coursetag_get_tags() has 'optional' used as if its a keyword rather than part of a sentence, perhaps an improvement to the sentence structure, capitalisation and general puctuation etc here? or you can always ask Helen to review the language.
          • above point applies to other functions in patch too.
          • coursetag_sort() : "@access private" where function is not defined as private, perhaps we need to sort this out and make it clearer? add a"do not use!" instead of 'only' ? discussion needed.
          • coursetag_delete_course_tags() $showfeedback has double 'the'
          • coursetabslib.php : coursetag_rss_feeds() & coursetag_get_official_keywords() are commented but their doc blocks have been modified into block comments. this seems ok, but maybe we should discuss and share that this is how we should do it if and when commenting. I would say that commented functions should really be removed. (this may be a separate issue/subtask)
          • tag/lib.php tag_type_set() description describes moodle 1.9 .
          • tag_description_set() "* @param string $description the description" could be "The tag's description string to be set."
          • personally i prefer the argument related phpdocs (@params/@return) to be spaced away from other (@package) phpdoc tags for easier source code reading. i don't think there is any rule though.

          Generally theres a smatterring of sentence structure that needs tidying up.

          Thats it for my first phpdoc review! re-opening, feel free to discuss any of the above.

          cheers,
          Aparup

          Show
          Aparup Banerjee added a comment - Hi Gerry, I've attached a smurf.xml here to the issue. Please have a look through it: formatting : we need to remove the trailing whitespaces. I always use 'git diff' and it shows them to me in red. phpdoc : theres a few functions in there that have incomplete. coursetagslib.php line 34 : param description could use a description of the variable, suggest something like: "A course id. 0 means all courses." We need to bear in mind that this might be the first function a developer might be trying to use. coursetagslib.php line 36 : Also i'm thinking specifying $tagtype types there would easily be obsolete as a description once types change, i would suggest 'The type of tag, empty string returns all types' or perhaps a @see is better here? The function doesn't limit the range of types used. coursetag_get_tags() has 'optional' used as if its a keyword rather than part of a sentence, perhaps an improvement to the sentence structure, capitalisation and general puctuation etc here? or you can always ask Helen to review the language. above point applies to other functions in patch too. coursetag_sort() : "@access private" where function is not defined as private, perhaps we need to sort this out and make it clearer? add a"do not use!" instead of 'only' ? discussion needed. coursetag_delete_course_tags() $showfeedback has double 'the' coursetabslib.php : coursetag_rss_feeds() & coursetag_get_official_keywords() are commented but their doc blocks have been modified into block comments. this seems ok, but maybe we should discuss and share that this is how we should do it if and when commenting. I would say that commented functions should really be removed. (this may be a separate issue/subtask) tag/lib.php tag_type_set() description describes moodle 1.9 . tag_description_set() "* @param string $description the description" could be "The tag's description string to be set." personally i prefer the argument related phpdocs (@params/@return) to be spaced away from other (@package) phpdoc tags for easier source code reading. i don't think there is any rule though. Generally theres a smatterring of sentence structure that needs tidying up. Thats it for my first phpdoc review! re-opening, feel free to discuss any of the above. cheers, Aparup
          Hide
          Gerard Caulfield added a comment - - edited

          Regarding
          $tagtype types, I agree using @see would be good however there is nothing to "see" as these types are not defined in the code. I have tried to update the code so it's less hostile to updates. I've also added a comment about this to the Tag API overhaul ticket MDL-31090

          As for "@access private", this is different from private methods: http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.access.pkg.html
          There was a conversation at the time about the use of @access private on functions such as these, however unfortunately it was not documented in the coding style.

          coursetabslib.php : coursetag_rss_feeds() & coursetag_get_official_keywords() are commented but their doc blocks have been modified into block comments. this seems ok, but maybe we should discuss and share that this is how we should do it if and when commenting. I would say that commented functions should really be removed. (this may be a separate issue/subtask)

          I agree about the commented out code being deleted. I've created an MDL for it MDL-31608.

          personally i prefer the argument related phpdocs (@params/@return) to be spaced away from other (@package) phpdoc tags for easier source code reading. i don't think there is any rule though.

          I don't mind either way, however it's not in the standard and if we want to make it part of the standard then it should be required of all the doc issues and not just this one. I think it's far more important to get these changes through so that developers can start using the improved documentation.

          I've made changes to everything else you mentioned. Thanks.

          Show
          Gerard Caulfield added a comment - - edited Regarding $tagtype types, I agree using @see would be good however there is nothing to "see" as these types are not defined in the code. I have tried to update the code so it's less hostile to updates. I've also added a comment about this to the Tag API overhaul ticket MDL-31090 As for "@access private", this is different from private methods: http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.access.pkg.html There was a conversation at the time about the use of @access private on functions such as these, however unfortunately it was not documented in the coding style. coursetabslib.php : coursetag_rss_feeds() & coursetag_get_official_keywords() are commented but their doc blocks have been modified into block comments. this seems ok, but maybe we should discuss and share that this is how we should do it if and when commenting. I would say that commented functions should really be removed. (this may be a separate issue/subtask) I agree about the commented out code being deleted. I've created an MDL for it MDL-31608 . personally i prefer the argument related phpdocs (@params/@return) to be spaced away from other (@package) phpdoc tags for easier source code reading. i don't think there is any rule though. I don't mind either way, however it's not in the standard and if we want to make it part of the standard then it should be required of all the doc issues and not just this one. I think it's far more important to get these changes through so that developers can start using the improved documentation. I've made changes to everything else you mentioned. Thanks.
          Hide
          Aparup Banerjee added a comment -

          Hi Gerry,
          thanks for those changes.
          i've got another smurf @ http://stronk7.doesntexist.com/view/prehecker/job/Precheck%20remote%20branch/101/artifact/work/smurf.xml

          It seems @access is being picked out in that smurf - 'Not recommended phpdocs tag @access used', i think we need to clarify in what situations we could use @access (or not at all?) in documentation. imo it does make sense to have it to remove unnecessary generated phpdoc clutter from an API. However it seems thats what we're using @category instead to explicitly pick out what we want to have generated - we should discuss/clarify this overlap of @category vs @access for generating phpdocs.

          http://moodle.org/mod/cvsadmin/view.php?chatsearch=%40access (no real definitive chat found yet)

          Show
          Aparup Banerjee added a comment - Hi Gerry, thanks for those changes. i've got another smurf @ http://stronk7.doesntexist.com/view/prehecker/job/Precheck%20remote%20branch/101/artifact/work/smurf.xml It seems @access is being picked out in that smurf - 'Not recommended phpdocs tag @access used', i think we need to clarify in what situations we could use @access (or not at all?) in documentation. imo it does make sense to have it to remove unnecessary generated phpdoc clutter from an API. However it seems thats what we're using @category instead to explicitly pick out what we want to have generated - we should discuss/clarify this overlap of @category vs @access for generating phpdocs. http://moodle.org/mod/cvsadmin/view.php?chatsearch=%40access (no real definitive chat found yet)
          Hide
          Aparup Banerjee added a comment -

          thanks for adding : http://docs.moodle.org/dev/Coding_style#.40access

          This has been integrated into master to share with developers all around!

          Show
          Aparup Banerjee added a comment - thanks for adding : http://docs.moodle.org/dev/Coding_style#.40access This has been integrated into master to share with developers all around!
          Hide
          Michael de Raadt added a comment -

          Test passed Docs look good.

          Show
          Michael de Raadt added a comment - Test passed Docs look good.
          Hide
          Gerard Caulfield added a comment -

          Thanks everybody.

          Show
          Gerard Caulfield added a comment - Thanks everybody.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks!

          Closing as fixed, heading to zzzZZZzzz, niao

          Show
          Eloy Lafuente (stronk7) added a comment - It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks! Closing as fixed, heading to zzzZZZzzz, niao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: