Moodle
  1. Moodle
  2. MDL-30975

log API, check and update DocBlock

    Details

    • Rank:
      37379

      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.

        Activity

        Hide
        Ankit Agarwal added a comment -

        will update the 22 branch once this is reviewed
        Thanks

        Show
        Ankit Agarwal added a comment - will update the 22 branch once this is reviewed Thanks
        Hide
        David Mudrak 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
        David Mudrak 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
        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
        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 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 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
        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
        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
        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
        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 Agarwal added a comment -

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

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

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

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

        rebased
        Thanks

        Show
        Ankit Agarwal added a comment - rebased Thanks
        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 -

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

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

        Show
        Ankit Agarwal added a comment - Hi Eloy, Core_log is correct Core_lib is now changed to core. Also rebased branch. Thanks
        Hide
        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
        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
        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
        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
        Rajesh Taneja added a comment -

        Frankenstyle page updated to reflect core_log.

        Thanks Eloy, for pointing that.

        Show
        Rajesh Taneja added a comment - Frankenstyle page updated to reflect core_log. Thanks Eloy, for pointing that.
        Hide
        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
        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 Agarwal added a comment -

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

        Show
        Ankit Agarwal added a comment - Adding Rajesh and Martin as watcher to comment and make a decision on this Thanks
        Hide
        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
        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
        Martin Dougiamas added a comment -

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

        @package core
        @category log

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

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

        Show
        Ankit Agarwal added a comment - all instances of core_log changed to core. Submitting for integration Thanks
        Hide
        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
        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
        Eloy Lafuente (stronk7) added a comment -

        nobody tested this

        Show
        Eloy Lafuente (stronk7) added a comment - nobody tested this
        Hide
        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
        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: