Moodle
  1. Moodle
  2. MDL-30976

navigation API, phpdocs and devdocs

    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, Navigation
    • Labels:
    • Rank:
      37380

      Description

      Check and update documentation, so that it should comply with moodle coding guidelines.
      Following needs to be updated/checked for navigation 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. MDL-30976_smurf.xml
        9 kB
        Eloy Lafuente (stronk7)

        Issue Links

          Activity

          Hide
          Adrian Greeve added a comment -

          Submitting for peer review so that I can get some feedback and fix up what I have missed.

          Show
          Adrian Greeve added a comment - Submitting for peer review so that I can get some feedback and fix up what I have missed.
          Hide
          David Mudrak added a comment -

          Is "@category navigation_node" valid API reference?

          Show
          David Mudrak added a comment - Is "@category navigation_node" valid API reference?
          Hide
          Rajesh Taneja added a comment -

          Hello Adrian,
          Few things you might want to consider:

          1. category should be navigation only.
          2. You should have description for the params "lib/navigationlib.php:: line - 855 :: @param mixed $type"
          3. @see forum_extend_settings_navigation() should be @see class::function()

          Rest looks Good

          Show
          Rajesh Taneja added a comment - Hello Adrian, Few things you might want to consider: category should be navigation only. You should have description for the params "lib/navigationlib.php:: line - 855 :: @param mixed $type" @see forum_extend_settings_navigation() should be @see class::function() Rest looks Good
          Hide
          Adrian Greeve added a comment -

          Thanks David and Rajesh.

          I changed the category to navigation
          I also fixed up the parameter description.
          forum_extend_settings_navigation() isn't part of a class, but I've fixed up other areas where the class section was missing.

          Thanks again,

          Adrian.

          Show
          Adrian Greeve added a comment - Thanks David and Rajesh. I changed the category to navigation I also fixed up the parameter description. forum_extend_settings_navigation() isn't part of a class, but I've fixed up other areas where the class section was missing. Thanks again, Adrian.
          Hide
          Sam Hemelryk added a comment -

          Hi Adrian,

          Just as a heads up there was a navigation issue integrated last week that led to argument changes in 2.3.
          Eloy released last night/this morning so it may pay to rebase your stuff if you haven't already in case there are conflicts.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Adrian, Just as a heads up there was a navigation issue integrated last week that led to argument changes in 2.3. Eloy released last night/this morning so it may pay to rebase your stuff if you haven't already in case there are conflicts. Cheers Sam
          Hide
          Adrian Greeve added a comment -

          Hi Sam,

          Thanks for the warning. I've rebased my stuff and pushed it back to github.

          Show
          Adrian Greeve added a comment - Hi Sam, Thanks for the warning. I've rebased my stuff and pushed it back to github.
          Hide
          Sam Hemelryk added a comment -

          Hi Adrian,

          I've just been looking at this now and here is my feedback on the phpdoc changes.

          1. blocks/navigation/* @package should be ''block_navigation'' not ''blocks_navigation'' (with plugins if you're not sure check version.php for >component)
          2. block_navigation class:
            • properties could do with a comment about what they are there for.
            • instance_can_be_docked missing phpdoc block
            • get_javascript_required missing phpdoc block
            • get_content missing @return
            • html_attributes @see should be class::method not class->method
            • trim method missing @param
          3. navigationlib.php
            • navigation_node::add_node incorrect @param
            • navigaiton_node::find_all_of_type typo in @param (on of navigation_node...)
            • global_navigation @links in class docblock could be tidied up
            • global_navigation properties need comments about what each is for.
            • global_navigation::load_all_categories return is navigation_node|void
            • global_navigation::load_section_activities incorrect @params
            • global_navigation_for_ajax properties need docblocks
            • global_navigation_for_ajax constructor $page is a moodle_page object
            • navbar properties need comments
            • navbar::get needs docblock
            • settings_navigation properties need comments
            • settings_navigation::add incorrect @params
            • same for prepend
            • navigation_json properties need comment
            • navigation_cache properties need comment

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Adrian, I've just been looking at this now and here is my feedback on the phpdoc changes. blocks/navigation/* @package should be ''block_navigation'' not ''blocks_navigation'' (with plugins if you're not sure check version.php for >component) block_navigation class: properties could do with a comment about what they are there for. instance_can_be_docked missing phpdoc block get_javascript_required missing phpdoc block get_content missing @return html_attributes @see should be class::method not class->method trim method missing @param navigationlib.php navigation_node::add_node incorrect @param navigaiton_node::find_all_of_type typo in @param (on of navigation_node...) global_navigation @links in class docblock could be tidied up global_navigation properties need comments about what each is for. global_navigation::load_all_categories return is navigation_node|void global_navigation::load_section_activities incorrect @params global_navigation_for_ajax properties need docblocks global_navigation_for_ajax constructor $page is a moodle_page object navbar properties need comments navbar::get needs docblock settings_navigation properties need comments settings_navigation::add incorrect @params same for prepend navigation_json properties need comment navigation_cache properties need comment Cheers Sam
          Hide
          Adrian Greeve added a comment -

          Thanks Sam for the review. I made the changes that you suggested. Moving onto the next step.

          Show
          Adrian Greeve added a comment - Thanks Sam for the review. I made the changes that you suggested. Moving onto the next step.
          Hide
          Sam Hemelryk added a comment -

          Hi Adrian, if all changes have now been made feel free to put this up for integration when you want.

          Show
          Sam Hemelryk added a comment - Hi Adrian, if all changes have now been made feel free to put this up for integration when you want.
          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 -

          Reopening, contains invalid whitespace and some invalid apis, packages (based on the checker).

          Show
          Eloy Lafuente (stronk7) added a comment - Reopening, contains invalid whitespace and some invalid apis, packages (based on the checker).
          Hide
          Adrian Greeve added a comment -

          I made the changes as per the smurf file. Though I couldn't find out what the package name for the navigation files should be under the blocks directory.
          I asked Rajesh and he said that it should be block_navigation.
          If you could please let me know what the package name should be please.

          Thanks!

          Show
          Adrian Greeve added a comment - I made the changes as per the smurf file. Though I couldn't find out what the package name for the navigation files should be under the blocks directory. I asked Rajesh and he said that it should be block_navigation. If you could please let me know what the package name should be please. Thanks!
          Hide
          Rajesh Taneja added a comment -

          Navigation is a block, and hence as per Frankenstyle, it should be block_navigation.

          Show
          Rajesh Taneja added a comment - Navigation is a block, and hence as per Frankenstyle , it should be block_navigation.
          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
          Adrian Greeve added a comment -

          Re-based and ready to go.

          Show
          Adrian Greeve added a comment - Re-based and ready to go.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've added one extra commit fixing a bunch of incorrect inline @see tags, replacing them by the correct @link ones.

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - I've added one extra commit fixing a bunch of incorrect inline @see tags, replacing them by the correct @link ones. 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 -

          Well,

          I wish I said it every time
          you do the things you do.
          You always lend a helping hand,
          and I'm filled with gratitude.

          You are strong and generous
          for each and everyone one of us.
          I am eternally grateful,
          I cannot say thanks enough.

          Sorry for the (un)cool bit above, lol. Closing this as fixed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, I wish I said it every time you do the things you do. You always lend a helping hand, and I'm filled with gratitude. You are strong and generous for each and everyone one of us. I am eternally grateful, I cannot say thanks enough. Sorry for the (un)cool bit above, lol. Closing this as fixed. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: