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:

      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.

        Gliffy Diagrams

        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: