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

time API, check and update DocBlock

    Details

      Description

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

          Issue Links

            Activity

            rajeshtaneja Rajesh Taneja created issue -
            rajeshtaneja Rajesh Taneja made changes -
            Field Original Value New Value
            Link This issue has a clone MDL-30992 [ MDL-30992 ]
            rajeshtaneja Rajesh Taneja made changes -
            Link This issue has a clone MDL-30992 [ MDL-30992 ]
            rajeshtaneja Rajesh Taneja made changes -
            Description Check and update documentation, so that it should comply with [moodle coding guidelines|http://docs.moodle.org/dev/Coding_style#Files].
            Following needs to be updated/checked for tag api
            # DocBlock for page and functions.
            # All the files should be checked/updated.

            Note: You can create sub-tasks, so as to avoid bulk integration.
            Check and update documentation, so that it should comply with [moodle coding guidelines|http://docs.moodle.org/dev/Coding_style#Files].
            Following needs to be updated/checked for time api
            # DocBlock for page and functions.
            # All the files should be checked/updated.

            Note: You can create sub-tasks, so as to avoid bulk integration.
            Component/s Administration [ 10050 ]
            Component/s Tags [ 10220 ]
            rajeshtaneja Rajesh Taneja made changes -
            Link This issue is a clone of MDL-30994 [ MDL-30994 ]
            rajeshtaneja Rajesh Taneja made changes -
            Link This issue is a clone of MDL-30994 [ MDL-30994 ]
            rajeshtaneja Rajesh Taneja made changes -
            Component/s Documentation [ 10073 ]
            rajeshtaneja Rajesh Taneja made changes -
            Assignee Rajesh Taneja [ rajeshtaneja ] moodle.com [ moodle.com ]
            salvetore Michael de Raadt made changes -
            Affects Version/s STABLE backlog [ 10463 ]
            Affects Version/s 2.2 [ 10656 ]
            Affects Version/s 2.3 [ 10657 ]
            Labels triaged
            Fix Version/s 2.2.1 [ 11456 ]
            Fix Version/s 2.2 [ 10656 ]
            salvetore Michael de Raadt made changes -
            Assignee moodle.com [ moodle.com ] Rajesh Taneja [ rajeshtaneja ]
            salvetore Michael de Raadt made changes -
            Affects Version/s Docs Sprint [ 11551 ]
            Affects Version/s STABLE backlog [ 10463 ]
            salvetore Michael de Raadt made changes -
            Affects Version/s 2.2 [ 10656 ]
            Affects Version/s 2.2.1 [ 11456 ]
            Affects Version/s Docs Sprint [ 11551 ]
            Fix Version/s Docs Sprint [ 11551 ]
            Fix Version/s 2.2 [ 10656 ]
            Fix Version/s 2.2.1 [ 11456 ]
            rajeshtaneja Rajesh Taneja made changes -
            Parent MDL-30971 [ 50000 ]
            Issue Type Sub-task [ 5 ] Task [ 3 ]
            rajeshtaneja Rajesh Taneja made changes -
            Status Open [ 1 ] Development in progress [ 3 ]
            rajeshtaneja Rajesh Taneja made changes -
            Pull Master Diff URL https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-30993
            Pull Master Branch wip-mdl-30993
            Pull from Repository git://github.com/rajeshtaneja/moodle.git
            rajeshtaneja Rajesh Taneja made changes -
            Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
            salvetore Michael de Raadt made changes -
            Original Estimate 0 minutes [ 0 ]
            Remaining Estimate 0 minutes [ 0 ]
            Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
            Peer reviewer salvetore
            Hide
            salvetore Michael de Raadt added a comment -

            Omm nomm nomm. I eat documentation for breakfast.

            I'm seeing a bit of variety for between people indenting when doc tag comments break over lines. Also, different people are aligning tag values/comments differently; some with alignment, some removing all alignment.

            Some comments:

            • Lines 1825, 1942, 2012, 2063, 2084, 2141, 2168, 2197, could benefit from more explanation of possible values
            • Lines 1825, 1942 are inconsistent in its use of mixed

            Looks pretty good.

            Show
            salvetore Michael de Raadt added a comment - Omm nomm nomm. I eat documentation for breakfast. I'm seeing a bit of variety for between people indenting when doc tag comments break over lines. Also, different people are aligning tag values/comments differently; some with alignment, some removing all alignment. Some comments: Lines 1825, 1942, 2012, 2063, 2084, 2141, 2168, 2197, could benefit from more explanation of possible values Lines 1825, 1942 are inconsistent in its use of mixed Looks pretty good.
            salvetore Michael de Raadt made changes -
            Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for the feedback Michael,
            Branches updated with your feedback. Can you please peer review this for me.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for the feedback Michael, Branches updated with your feedback. Can you please peer review this for me.
            rajeshtaneja Rajesh Taneja made changes -
            Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
            rajeshtaneja Rajesh Taneja made changes -
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for the feedback Michael,
            I have added link for timezone and pushing this for integration review.

            Still working on doc page.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for the feedback Michael, I have added link for timezone and pushing this for integration review. Still working on doc page.
            rajeshtaneja Rajesh Taneja made changes -
            Status Waiting for peer review [ 10012 ] Waiting for integration review [ 10010 ]
            Documentation link http://docs.moodle.org/dev/Time_API
            stronk7 Eloy Lafuente (stronk7) made changes -
            Currently in integration Yes [ 10041 ]
            Hide
            rajeshtaneja Rajesh Taneja added a comment -
            Show
            rajeshtaneja Rajesh Taneja added a comment - Updated doc page http://docs.moodle.org/dev/Time_API
            samhemelryk Sam Hemelryk made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator samhemelryk
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Raj,

            Looking for comment on the following things before this gets integrated/rejected.

            1. @package. phpdoc manual states that @package is only applicable to procedural pages and classes. I don't think we should be setting this on functions, I haven't heard any discussion on this and/or modifying package x to support such a thing. Given we have category now for that sort of hackery however it gets my -1 on adding any @package to functions.
            2. @access. I'm not sure about using `@access public` on either classes or functions. This information is redundant as functions and classes are public by default and are only private/protected because of early coding practises or bad design. Personally I think we shouldn't use @access public. I think we should only use @access protected, and @access private.
            3. I don't like the forced alignment that is going on with @package, @category, etc where you've got an @xxx multiple spaces and then the value. I remember Sam Marshall wrote a great article about the annoyances that sort of alignment causes and given that we don't do a similar thing with other @ declarations like @param I think we shouldn't encourage such alignment (I know there are cases of it but in general we don't align vars like that because of the misalignment caused as code changes). Found the post: http://learn.open.ac.uk/mod/oublog/viewpost.php?post=74901 (gets syndicated everywhere as well of course). I'm up for convincing otherwise on this one, presently my personal preference is as stated, but really I think we should just be clear on what we are doing and try to do it consistently.
            4. @return mixed on get_user_timezone could be improved.
            5. @return for get_timezone_record shouldn't use object, should use stdClass. Remember the object class has been deprecated since 2.0 (git show 4f92410fb5)

            Just noting:

            • The third argument to the get_record call in get_timezone_record is incorrect. true == IGNORE_MULTIPLE and should be changed at some point. Will create an issue for that.

            Let me know your thoughts.
            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Raj, Looking for comment on the following things before this gets integrated/rejected. @package. phpdoc manual states that @package is only applicable to procedural pages and classes. I don't think we should be setting this on functions, I haven't heard any discussion on this and/or modifying package x to support such a thing. Given we have category now for that sort of hackery however it gets my -1 on adding any @package to functions. @access. I'm not sure about using `@access public` on either classes or functions. This information is redundant as functions and classes are public by default and are only private/protected because of early coding practises or bad design. Personally I think we shouldn't use @access public. I think we should only use @access protected, and @access private. I don't like the forced alignment that is going on with @package, @category, etc where you've got an @xxx multiple spaces and then the value. I remember Sam Marshall wrote a great article about the annoyances that sort of alignment causes and given that we don't do a similar thing with other @ declarations like @param I think we shouldn't encourage such alignment (I know there are cases of it but in general we don't align vars like that because of the misalignment caused as code changes). Found the post: http://learn.open.ac.uk/mod/oublog/viewpost.php?post=74901 (gets syndicated everywhere as well of course). I'm up for convincing otherwise on this one, presently my personal preference is as stated, but really I think we should just be clear on what we are doing and try to do it consistently. @return mixed on get_user_timezone could be improved. @return for get_timezone_record shouldn't use object, should use stdClass. Remember the object class has been deprecated since 2.0 (git show 4f92410fb5) Just noting: The third argument to the get_record call in get_timezone_record is incorrect. true == IGNORE_MULTIPLE and should be changed at some point. Will create an issue for that. Let me know your thoughts. Cheers Sam
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for the feedback Sam,

            1. You are right, but we have agreed upon using @package tag for functions, as functions are scattered in different files for CORE_API, hence for this project we made this as exception to group these using package tag.
            2. I agree with this information is redundant for public, but I don't see any harm in adding this information.
            3. Not sure of this, will check with you before changing/reverting
            4. will update this
            5. Will update this
            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for the feedback Sam, You are right, but we have agreed upon using @package tag for functions, as functions are scattered in different files for CORE_API, hence for this project we made this as exception to group these using package tag. I agree with this information is redundant for public, but I don't see any harm in adding this information. Not sure of this, will check with you before changing/reverting will update this Will update this
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for discussing this Sam, Summarizing it below:

            1. @package is used in functions where function can't be grouped under @package of file/class. Example is moodlelib.php having preference_api, time api etc. functions which can't be grouped under single package.
            2. @acess public is no harm but is a redundant information and should not be used. Saying so, @acess private and @access protected is acceptable.
            3. Forced alignment of tags should be consistent in docblock. It's up-to the developer to go either way (aligned or single space), but it should be consistent for the whole docblock.
            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for discussing this Sam, Summarizing it below: @package is used in functions where function can't be grouped under @package of file/class. Example is moodlelib.php having preference_api, time api etc. functions which can't be grouped under single package. @acess public is no harm but is a redundant information and should not be used. Saying so, @acess private and @access protected is acceptable. Forced alignment of tags should be consistent in docblock. It's up-to the developer to go either way (aligned or single space), but it should be consistent for the whole docblock.
            rajeshtaneja Rajesh Taneja made changes -
            Link This issue testing discovered MDL-31208 [ MDL-31208 ]
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Branches updated with your suggestions
            Also, created issue for third argument to the get_record

            Show
            rajeshtaneja Rajesh Taneja added a comment - Branches updated with your suggestions Also, created issue for third argument to the get_record
            Hide
            mudrd8mz David Mudrak added a comment -

            I am especially happy about #3 in the summary above. Thanks guys.

            Show
            mudrd8mz David Mudrak added a comment - I am especially happy about #3 in the summary above. Thanks guys.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Stopping review this issue will be held back til next week.

            Show
            samhemelryk Sam Hemelryk added a comment - Stopping review this issue will be held back til next week.
            samhemelryk Sam Hemelryk made changes -
            Status Integration review in progress [ 10004 ] Waiting for integration review [ 10010 ]
            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
            stronk7 Eloy Lafuente (stronk7) made changes -
            Currently in integration Yes [ 10041 ]
            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
            rajeshtaneja Rajesh Taneja made changes -
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            branch re-based.

            Show
            rajeshtaneja Rajesh Taneja added a comment - branch re-based.
            stronk7 Eloy Lafuente (stronk7) made changes -
            Currently in integration Yes [ 10041 ]
            samhemelryk Sam Hemelryk made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Raj,

            Reopening this, just been going through the ideas for @package and @package for this should be core not core_time.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Raj, Reopening this, just been going through the ideas for @package and @package for this should be core not core_time. Cheers Sam
            samhemelryk Sam Hemelryk made changes -
            Status Integration review in progress [ 10004 ] Reopened [ 4 ]
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Sam,
            I have updated @package. Pushing it back for your review.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Sam, I have updated @package. Pushing it back for your review.
            rajeshtaneja Rajesh Taneja made changes -
            Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
            samhemelryk Sam Hemelryk made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Marked as integrated (master). Thanks Sam et Rajesh!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Marked as integrated (master). Thanks Sam et Rajesh!
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Affects Version/s 2.3 [ 10657 ]
            Fix Version/s 2.3 [ 10657 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Tester stronk7
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Passing, just re-reviewed that:

            • phpdocs look correct.
            • all the changes are phpdocs.
            • it didn't break any check in the CI server.

            So that should be enough. Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Passing, just re-reviewed that: phpdocs look correct. all the changes are phpdocs. it didn't break any check in the CI server. So that should be enough. Ciao
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Your nice code represents only 1/46 of the issues that have been sent upstream this week, so thanks, but not many.

            Nah, joking, many thanks! Closing this a fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Your nice code represents only 1/46 of the issues that have been sent upstream this week, so thanks, but not many. Nah, joking, many thanks! Closing this a fixed, ciao
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            Integration date 27/Jan/12
            stronk7 Eloy Lafuente (stronk7) made changes -
            Fix Version/s Docs Sprint [ 11551 ]

              People

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

                Dates

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