Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30991

rss API, check and update DocBlock

    Details

    • Type: Task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.2.1
    • Fix Version/s: 2.3
    • Component/s: Documentation, RSS
    • Labels:

      Description

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

          Attachments

            Activity

            Hide
            dougiamas Martin Dougiamas added a comment -

            Can you post your git links please?

            Show
            dougiamas Martin Dougiamas added a comment - Can you post your git links please?
            Hide
            phalacee Jason Fowler added a comment -

            Updated github repos

            Show
            phalacee Jason Fowler added a comment - Updated github repos
            Hide
            phalacee Jason Fowler added a comment -

            will port to 2.2 after peer review

            Show
            phalacee Jason Fowler added a comment - will port to 2.2 after peer review
            Hide
            phalacee Jason Fowler added a comment -

            2.2 done now, as it was a clean cherry-pick

            Show
            phalacee Jason Fowler added a comment - 2.2 done now, as it was a clean cherry-pick
            Hide
            salvetore Michael de Raadt added a comment -

            Looks like you've had to add more text than most others completing this task.

            Some comments:

            • blog/rsslib.php
              • No blank line before start of GPL statement
              • Lines 36, 71, 95, 282, 300, is the description of $tagid correct? It looks like an index rather than a name. Is it an int or a string? I couldn't find a call to it.
              • Line 157, the args should probably be explained, or a link should be used.
              • Line 296, shift "to file" to the end of the sentence, or add commas
            • lib/rsslib.php
              • Line 52, the function is returning, not printing the link?
              • Could add a @see to rss_get_filename() as per line formerly at 86
              • Lines 156, 157, perhaps "eg" rather than "ie"
              • Aside: Line 350, not the most useless function I've seen today, but close
              • Line 423, blank line needed after description
              • Line 471, "whike" to "whole" (keyboard offset, right hand, one key)
              • Line 493, add @copyright for authors as previously listed
            • mod/data/rsslib.php
              • Line 32, the args should probably be explained, or a link should be used.
              • Aside: data_rss_newstuff() - very non-descript
            • mod/forum/rsslib.php
              • @globals are optional, but are allowed, perhaps they need not be removed
              • Line 31, the args should probably be explained, or a link should be used.
              • Lines 90, 120, 142, 205, is there a more specific class than stdClass for a forum object?
              • Line 254, description needed
              • Line 293 @todo in a phpdoc tag should be lower-case. As you have this below, remove it from the phpdoc (and blank line above)
            • mod/glossary/rsslib.php
              • Line 29, blank line needed after description
              • Line 31, the args should probably be explained, or a link should be used.
              • Line 135, capitalise "The"
            • rss/renderer.php
              • Lines 23, 31, 40, 55, blank lines needed after descriptions

            A lot of little comments there. You've done a lot of work to complete this.

            Show
            salvetore Michael de Raadt added a comment - Looks like you've had to add more text than most others completing this task. Some comments: blog/rsslib.php No blank line before start of GPL statement Lines 36, 71, 95, 282, 300, is the description of $tagid correct? It looks like an index rather than a name. Is it an int or a string? I couldn't find a call to it. Line 157, the args should probably be explained, or a link should be used. Line 296, shift "to file" to the end of the sentence, or add commas lib/rsslib.php Line 52, the function is returning, not printing the link? Could add a @see to rss_get_filename() as per line formerly at 86 Lines 156, 157, perhaps "eg" rather than "ie" Aside: Line 350, not the most useless function I've seen today, but close Line 423, blank line needed after description Line 471, "whike" to "whole" (keyboard offset, right hand, one key) Line 493, add @copyright for authors as previously listed mod/data/rsslib.php Line 32, the args should probably be explained, or a link should be used. Aside: data_rss_newstuff() - very non-descript mod/forum/rsslib.php @globals are optional, but are allowed, perhaps they need not be removed Line 31, the args should probably be explained, or a link should be used. Lines 90, 120, 142, 205, is there a more specific class than stdClass for a forum object? Line 254, description needed Line 293 @todo in a phpdoc tag should be lower-case. As you have this below, remove it from the phpdoc (and blank line above) mod/glossary/rsslib.php Line 29, blank line needed after description Line 31, the args should probably be explained, or a link should be used. Line 135, capitalise "The" rss/renderer.php Lines 23, 31, 40, 55, blank lines needed after descriptions A lot of little comments there. You've done a lot of work to complete this.
            Hide
            phalacee Jason Fowler added a comment - - edited

            For line 282 $tagid is definitely a string,
            http://jason.moodle.local/master/rss/file.php?file=/1/e108c20005519e2c5c56b8f44ef0fee6/blog/user/2/rss.xml
            in the case of the URL above 'rss' in rss.xml is the the $tagid

            It was the only place I could actually trace the variable for, so I am unsure about the others

            Show
            phalacee Jason Fowler added a comment - - edited For line 282 $tagid is definitely a string, http://jason.moodle.local/master/rss/file.php?file=/1/e108c20005519e2c5c56b8f44ef0fee6/blog/user/2/rss.xml in the case of the URL above 'rss' in rss.xml is the the $tagid It was the only place I could actually trace the variable for, so I am unsure about the others
            Hide
            phalacee Jason Fowler added a comment -

            glossary_rss_get_feed(), forum_rss_get_feed() and data_rss_get_feed() are not used in any other area of the code, so I can't find out what the args are meant to be, and there is no where to link them to either

            Show
            phalacee Jason Fowler added a comment - glossary_rss_get_feed(), forum_rss_get_feed() and data_rss_get_feed() are not used in any other area of the code, so I can't find out what the args are meant to be, and there is no where to link them to either
            Hide
            phalacee Jason Fowler added a comment -

            I've made all the changes to the code that I can, based on the comments by Michael, and have responded to the others in comments above.

            Show
            phalacee Jason Fowler added a comment - I've made all the changes to the code that I can, based on the comments by Michael, and have responded to the others in comments above.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Moving all the phpdocs-related issues out from current integration, will be examined next week.

            Note that all these issues are going to be applied exclusively to master, that has been summarily decided by the integration team after hea(t)ring everybody.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Moving all the phpdocs-related issues out from current integration, will be examined next week. Note that all these issues are going to be applied exclusively to master, that has been summarily decided by the integration team after hea(t)ring everybody. Ciao
            Hide
            stronk7 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
            stronk7 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
            samhemelryk Sam Hemelryk added a comment -

            Hi Jason,

            Sending this back so that the following can be tidied up:

            1. rss/renderer.php @package should be core_rss, not core
            2. phpdoc block for core_rss_renderer has an empty line between the docblock and class (not sure if this is a problem or not but seeing as its going back please fix it up)
            3. core_rss_renderer::user_rss_token_box $token is a string not an object
            4. blog_rss_get_feed is not deprecated. It is a callback function and very much still in full use. Check out rss/file.php is retrieving content for the requested feed. (You'll also be able to work out the first 3 args in the $args array from looking there).
            5. lib/rsslib.php incorrect @param for rss_get_url
            6. lib/rsslib.php typo for @return for rss_add_items
            7. lib/rsslib.php @param for rss_geterrorxmlfile has args the wrong way round
            8. same as above for rss_add_enclosures
            9. data_rss_newstuff declares $data as object, data_rss_get_sql declares it as stdClass. It is a stdClass data_rss_newstuff needs to be updated
            10. Incorrect @params for forum_rss_get_feed
            11. glossary_rss_newstuff is using object as well and should be updated like data
            12. rss/file.php line 32 empty //
            13. @params for rss_error in rss/file.php are missing the type.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Jason, Sending this back so that the following can be tidied up: rss/renderer.php @package should be core_rss, not core phpdoc block for core_rss_renderer has an empty line between the docblock and class (not sure if this is a problem or not but seeing as its going back please fix it up) core_rss_renderer::user_rss_token_box $token is a string not an object blog_rss_get_feed is not deprecated. It is a callback function and very much still in full use. Check out rss/file.php is retrieving content for the requested feed. (You'll also be able to work out the first 3 args in the $args array from looking there). lib/rsslib.php incorrect @param for rss_get_url lib/rsslib.php typo for @return for rss_add_items lib/rsslib.php @param for rss_geterrorxmlfile has args the wrong way round same as above for rss_add_enclosures data_rss_newstuff declares $data as object, data_rss_get_sql declares it as stdClass. It is a stdClass data_rss_newstuff needs to be updated Incorrect @params for forum_rss_get_feed glossary_rss_newstuff is using object as well and should be updated like data rss/file.php line 32 empty // @params for rss_error in rss/file.php are missing the type. Cheers Sam
            Hide
            dougiamas Martin Dougiamas added a comment -

            Docs still need a lot of work too. The examples contain a little too much blog-specific code. Try and simplify them down to the bare essentials and mention the functions and files that plugins need to implement.

            This shouldn't be very hard. http://docs.moodle.org/dev/RSS_API

            Show
            dougiamas Martin Dougiamas added a comment - Docs still need a lot of work too. The examples contain a little too much blog-specific code. Try and simplify them down to the bare essentials and mention the functions and files that plugins need to implement. This shouldn't be very hard. http://docs.moodle.org/dev/RSS_API
            Hide
            phalacee Jason Fowler added a comment -

            Thanks for the feedback Sam, I've made the changes now, and will push them up shortly

            Show
            phalacee Jason Fowler added a comment - Thanks for the feedback Sam, I've made the changes now, and will push them up shortly
            Hide
            phalacee Jason Fowler added a comment -

            Hi Martin, I've tried to simplify the RSS API as best I can, based on my understanding of it. If you have some time, could you please take a look at it and make some suggestions on what else I can do to improve it?

            Show
            phalacee Jason Fowler added a comment - Hi Martin, I've tried to simplify the RSS API as best I can, based on my understanding of it. If you have some time, could you please take a look at it and make some suggestions on what else I can do to improve it?
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Jason, about the php-docs, everything looks correct, but there are some incorrect alignments of the "*" here and there.

            Take a look to the attached smurf.xml file (ignore line lengths and @global comments, just the "asterisk" and "whitespace" ones).

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Jason, about the php-docs, everything looks correct, but there are some incorrect alignments of the "*" here and there. Take a look to the attached smurf.xml file (ignore line lengths and @global comments, just the "asterisk" and "whitespace" ones). Ciao
            Hide
            phalacee Jason Fowler added a comment -

            The Smurf.xml is the rating one here ... can you please attach the RSS one for me to look over?

            Show
            phalacee Jason Fowler added a comment - The Smurf.xml is the rating one here ... can you please attach the RSS one for me to look over?
            Hide
            phalacee Jason Fowler added a comment -

            or better yet, please tell me how I can produce the file myself

            Show
            phalacee Jason Fowler added a comment - or better yet, please tell me how I can produce the file myself
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Jason,

            I've attached the smurf file for the result of the checks for this issue.
            The problems seems to have come about because the branch name contains the wrong MDL issue number and Eloy's Jenkins server is using the MDL issue number from the branch name.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Jason, I've attached the smurf file for the result of the checks for this issue. The problems seems to have come about because the branch name contains the wrong MDL issue number and Eloy's Jenkins server is using the MDL issue number from the branch name. Cheers Sam
            Hide
            phalacee Jason Fowler added a comment -

            Thanks Sam

            Show
            phalacee Jason Fowler added a comment - Thanks Sam
            Hide
            phalacee Jason Fowler added a comment -

            Fixes pushed now

            Show
            phalacee Jason Fowler added a comment - Fixes pushed now
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Jason this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Jason this has been integrated now.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Nobody tested this, passed!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Nobody tested this, passed!
            Hide
            stronk7 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
            stronk7 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:
                1 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12