Moodle
  1. Moodle
  2. MDL-30991

rss API, check and update DocBlock

    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, RSS
    • Labels:
    • Rank:
      37395

      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.

      1. rss.smurf.xml
        12 kB
        Sam Hemelryk
      2. smurf.xml
        6 kB
        Eloy Lafuente (stronk7)

        Activity

        Hide
        Martin Dougiamas added a comment -

        Can you post your git links please?

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

        Updated github repos

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

        will port to 2.2 after peer review

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

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

        Show
        Jason Fowler added a comment - 2.2 done now, as it was a clean cherry-pick
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Jason Fowler added a comment -

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

        Show
        Jason Fowler added a comment - Thanks for the feedback Sam, I've made the changes now, and will push them up shortly
        Hide
        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
        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
        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
        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
        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
        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
        Jason Fowler added a comment -

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

        Show
        Jason Fowler added a comment - or better yet, please tell me how I can produce the file myself
        Hide
        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
        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
        Jason Fowler added a comment -

        Thanks Sam

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

        Fixes pushed now

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

        Thanks Jason this has been integrated now.

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

        Nobody tested this, passed!

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

            Dates

            • Created:
              Updated:
              Resolved: