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

log 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 log 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
            ankit_frenz Ankit Agarwal added a comment -

            will update the 22 branch once this is reviewed
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - will update the 22 branch once this is reviewed Thanks
            Hide
            mudrd8mz David Mudrák added a comment -

            Hi Ankit. I did not do a full review, just spotted quickly that:

            • it seems that you left "@subpackage lib" in lib/datalib.php
            • get_logs() in lib/datalib.php does not return moodle_recordset instance - please read the code
            Show
            mudrd8mz David Mudrák added a comment - Hi Ankit. I did not do a full review, just spotted quickly that: it seems that you left "@subpackage lib" in lib/datalib.php get_logs() in lib/datalib.php does not return moodle_recordset instance - please read the code
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            In addition to what David spotted, here are few things:

            1. CFG is stdClass, so try use specific datatype which is known. In moodle, we use stdClass and not object.
            2. package and category should be added to function docblock
            3. @uses DAYSECS should be like @uses DAYSECS define for days in seconds.
            4. some of the params are not defined "report/log/lib.php :: report_log_can_access_user_report"
            Show
            rajeshtaneja Rajesh Taneja added a comment - In addition to what David spotted, here are few things: CFG is stdClass, so try use specific datatype which is known. In moodle, we use stdClass and not object. package and category should be added to function docblock @uses DAYSECS should be like @uses DAYSECS define for days in seconds. some of the params are not defined "report/log/lib.php :: report_log_can_access_user_report"
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi guys,
            Thanks for the suggestions.
            I have updated the patch.
            Also note that report/log and report/loglive are not a part of the core API (the functions in datalib.php), I just went ahead and documented them anyway.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi guys, Thanks for the suggestions. I have updated the patch. Also note that report/log and report/loglive are not a part of the core API (the functions in datalib.php), I just went ahead and documented them anyway. Thanks
            Hide
            dougiamas Martin Dougiamas added a comment -

            Ankit, well done, I had a quick skim and it looks like you tagged everything correctly.

            Can you put all the things you marked with "@category log" on http://docs.moodle.org/dev/Log_API and try to massage it into a document explaining how logs work and what the main functions available are?

            Show
            dougiamas Martin Dougiamas added a comment - Ankit, well done, I had a quick skim and it looks like you tagged everything correctly. Can you put all the things you marked with "@category log" on http://docs.moodle.org/dev/Log_API and try to massage it into a document explaining how logs work and what the main functions available are?
            Hide
            dougiamas Martin Dougiamas added a comment -

            Oops, I just noticed you left out mod/*/db/log.php files. Tag those as the

            @package mod_whatever
            @category log

            As they are good examples of implementations to look at.

            Show
            dougiamas Martin Dougiamas added a comment - Oops, I just noticed you left out mod/*/db/log.php files. Tag those as the @package mod_whatever @category log As they are good examples of implementations to look at.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Martin,
            Thanks for the feedback. I will take care of it.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Martin, Thanks for the feedback. I will take care of it. Thanks
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Ankit, does this need a peer-review still or can it be put up for integration now?

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Ankit, does this need a peer-review still or can it be put up for integration now?
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Sam,
            I guess it can be sent for integration since, Rajesh and Martin already had looked into this once.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Sam, I guess it can be sent for integration since, Rajesh and Martin already had looked into this once. Thanks
            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
            ankit_frenz Ankit Agarwal added a comment -

            rebased
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - rebased Thanks
            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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Ops, got really surprised viewing @package occurrences like core_log and core_lib. Are those correct? I think they aren't at all... and have checked it against last docs:

            http://docs.moodle.org/dev/Coding_style#.40package

            Was this reviewed against some old iteration of the rules or so? Confused, please clarify. I've asked in the HQ chat.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Ops, got really surprised viewing @package occurrences like core_log and core_lib. Are those correct? I think they aren't at all... and have checked it against last docs: http://docs.moodle.org/dev/Coding_style#.40package Was this reviewed against some old iteration of the rules or so? Confused, please clarify. I've asked in the HQ chat. Ciao
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Eloy,
            Core_log is correct Core_lib is now changed to core.
            Also rebased branch.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Eloy, Core_log is correct Core_lib is now changed to core. Also rebased branch. Thanks
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Sorry Ankit but... can you specify where it's said that "core_log" is a valid @package value? It's not in the list of plugins nor in the list of subsystems... so I don't get why it is valid (surely there is one reason, but I don't know about it, sorry, rejecting).

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Sorry Ankit but... can you specify where it's said that "core_log" is a valid @package value? It's not in the list of plugins nor in the list of subsystems... so I don't get why it is valid (surely there is one reason, but I don't know about it, sorry, rejecting).
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Hello Eloy,

            On core api page Logging API has been mentioned. Unfortunately it was missed on Frankenstyle page. I will request Martin to update this.

            AFAIK core_log is fine, it just got missed on Frankenstyle page.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Hello Eloy, On core api page Logging API has been mentioned. Unfortunately it was missed on Frankenstyle page . I will request Martin to update this. AFAIK core_log is fine, it just got missed on Frankenstyle page.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Frankenstyle page updated to reflect core_log.

            Thanks Eloy, for pointing that.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Frankenstyle page updated to reflect core_log. Thanks Eloy, for pointing that.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            NONONONONONONONONONO! :-P

            It's one API (not problem at all!) So can be used in @category (without "core_")

            But it's NOT a subsystem. So cannot be used in @package.

            Please don't add arbitrary subsystems at all!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - NONONONONONONONONONO! :-P It's one API (not problem at all!) So can be used in @category (without "core_") But it's NOT a subsystem. So cannot be used in @package. Please don't add arbitrary subsystems at all! Ciao
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Adding Rajesh and Martin as watcher to comment and make a decision on this
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Adding Rajesh and Martin as watcher to comment and make a decision on this Thanks
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Eloy,

            I am sorry Ankit, for pointing you in wrong direction.
            Eloy is right here, @package should be core and @category should be log.

            Will update doc after discussing with Martin.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Eloy, I am sorry Ankit, for pointing you in wrong direction. Eloy is right here, @package should be core and @category should be log. Will update doc after discussing with Martin.
            Hide
            dougiamas Martin Dougiamas added a comment -

            Yes, corrrrrrrect. It's not a true subsystem.

            @package core
            @category log

            Show
            dougiamas Martin Dougiamas added a comment - Yes, corrrrrrrect. It's not a true subsystem. @package core @category log
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            all instances of core_log changed to core.
            Submitting for integration
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - all instances of core_log changed to core. Submitting for integration Thanks
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, I've added one extra commit to add one missing @param to report_log_print_graph().

            Also, there are a lot of @global tags that, while not forbidden, I'm not 100% sure if they are the perfect way. But that does not prevent this to be integrated, thanks!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, I've added one extra commit to add one missing @param to report_log_print_graph(). Also, there are a lot of @global tags that, while not forbidden, I'm not 100% sure if they are the perfect way. But that does not prevent this to be integrated, thanks! Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            nobody tested this

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - nobody tested this
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks!

            Closing as fixed, heading to zzzZZZzzz, niao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks! Closing as fixed, heading to zzzZZZzzz, niao

              People

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

                Dates

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