Moodle
  1. Moodle
  2. MDL-30995

activity_completion API, check and update DocBlock

    Details

    • Rank:
      37399

      Description

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

        Hmm perhaps some confusion here... for some reason I though I had to do this, looks like you're already working on it though Ankit.
        If its of any use or you havn't started:

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hmm perhaps some confusion here... for some reason I though I had to do this, looks like you're already working on it though Ankit. If its of any use or you havn't started: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-30995-m23 https://github.com/samhemelryk/moodle/compare/MOODLE_22_STABLE...wip-MDL-30995-m22 Cheers Sam
        Hide
        Ankit Agarwal added a comment -

        Hi Sam,
        I guess Raj wanted you to work on conditional activities.

        Anyways thanks for the branches, that will help a lot.
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Sam, I guess Raj wanted you to work on conditional activities. Anyways thanks for the branches, that will help a lot. Thanks
        Hide
        Sam Hemelryk added a comment -

        Rebased.

        Show
        Sam Hemelryk added a comment - Rebased.
        Hide
        Michael de Raadt added a comment - - edited

        This looks like a significant piece of work.

        Looking at the files, my first question is: are there any functions related to completion in /lib outside of /completion that also need to be described?

        Some comments on the code:

        • lib/completion/completion_aggregation.php
          • The member variables and methods of completion_aggregation could keep their @access tags. Alternately, change the @var tags into one liners.
        • lib/completion/completion_completion.php
          • Same comment regarding @access tags, this seems to be the case in the remaining files
          • The boolean at lines 117, 220 should be bool
          • Although the indenting at line 219 is incorrect, we should avoid changes like this for now. Perhaps you can accumulate such changes in a separate issue.
        • lib/completion/completion_criteria.php
          • The additional check at line 27 should be done later
          • $data should be described at line 202
        • lib/completion/completion_criteria_activity.php
          • The description of $criteriatype at line 44 doesn't need the value there. Also, I'm not sure what happens later, but even though it is set with a constant, it might not stay the same, so the word constant might not be appropriate at line 43.
          • $data should be described at line 63
          • The mixed return type at line 117 should be stdClass|bool
          • The boolean at lines 140, 141 should be bool
          • Line 196 "user's" does not require an apostrophe
        • lib/completion/completion_criteria_completion.php
          • The boolean at lines 85, 128 should be bool
          • The change at line 176 is unnecessary
        • lib/completion/completion_criteria_course.php
          • $data should be described at line 64
          • The boolean at lines 101, 102 should be bool
        • lib/completion/completion_criteria_date.php
          • $data should be described at line 61
          • The boolean at lines 94, 95 should be bool
        • lib/completion/completion_criteria_duration.php
          • $data should be described at line 61
          • I'm going to stop reporting boolean->bool instances at this point. There may also be instances outside what you have edited that I am not finding. Perhaps you could do a global search an complete on all these files.
        • lib/completion/completion_criteria_grade.php
          • The parameter $params at line 51 could be described with possible key values. Perhaps this could link to docs for a more detailed example
          • $data should be described at line 63
          • The change at line 82 is unnecessary
          • $completion should be described at line 92
        • lib/completion/completion_criteria_role.php
          • $data should be described at line 61
          • The deletions after line 77 were unnecessary
          • The change at line 101 is unnecessary
        • lib/completion/completion_criteria_unenrol.php
          • $data should be described at line 61
        • lib/completion/cron.php is OK
        • lib/completion/data_object.php
          • The change at line 72 is unnecessary
          • The parameter $params at lines 116, 129, 143, 164 could be described with possible key values. Perhaps this could link to docs for a more detailed example
        • lib/completionlib.php
          • The word "activit" is missing a "y" at line 101
          • At lines 219, 479, 541, 622, 727, 808, 930, I don't think "cm_info" or "cm" are defined types (I only know this after reviewing someone else's code), just go with stdClass
          • The change at line 225 is unnecessary
          • The return type at line 283 should be "bool|completion_criteria_completion" and the fail state should be described above or in the return description
          • $item and $grade could be described at 1275 and 1276 respectively

        I kept getting lost in the files because they have similar filenames. My apologies if my comments don't appear in the correct files.

        There were a few changes to things like indenting outside of the doc blocks. We should avoid these for now. They can be created as separate issues if you believe they are significant, but most of them were not.

        Show
        Michael de Raadt added a comment - - edited This looks like a significant piece of work. Looking at the files, my first question is: are there any functions related to completion in /lib outside of /completion that also need to be described? Some comments on the code: lib/completion/completion_aggregation.php The member variables and methods of completion_aggregation could keep their @access tags. Alternately, change the @var tags into one liners. lib/completion/completion_completion.php Same comment regarding @access tags, this seems to be the case in the remaining files The boolean at lines 117, 220 should be bool Although the indenting at line 219 is incorrect, we should avoid changes like this for now. Perhaps you can accumulate such changes in a separate issue. lib/completion/completion_criteria.php The additional check at line 27 should be done later $data should be described at line 202 lib/completion/completion_criteria_activity.php The description of $criteriatype at line 44 doesn't need the value there. Also, I'm not sure what happens later, but even though it is set with a constant, it might not stay the same, so the word constant might not be appropriate at line 43. $data should be described at line 63 The mixed return type at line 117 should be stdClass|bool The boolean at lines 140, 141 should be bool Line 196 "user's" does not require an apostrophe lib/completion/completion_criteria_completion.php The boolean at lines 85, 128 should be bool The change at line 176 is unnecessary lib/completion/completion_criteria_course.php $data should be described at line 64 The boolean at lines 101, 102 should be bool lib/completion/completion_criteria_date.php $data should be described at line 61 The boolean at lines 94, 95 should be bool lib/completion/completion_criteria_duration.php $data should be described at line 61 I'm going to stop reporting boolean->bool instances at this point. There may also be instances outside what you have edited that I am not finding. Perhaps you could do a global search an complete on all these files. lib/completion/completion_criteria_grade.php The parameter $params at line 51 could be described with possible key values. Perhaps this could link to docs for a more detailed example $data should be described at line 63 The change at line 82 is unnecessary $completion should be described at line 92 lib/completion/completion_criteria_role.php $data should be described at line 61 The deletions after line 77 were unnecessary The change at line 101 is unnecessary lib/completion/completion_criteria_unenrol.php $data should be described at line 61 lib/completion/cron.php is OK lib/completion/data_object.php The change at line 72 is unnecessary The parameter $params at lines 116, 129, 143, 164 could be described with possible key values. Perhaps this could link to docs for a more detailed example lib/completionlib.php The word "activit" is missing a "y" at line 101 At lines 219, 479, 541, 622, 727, 808, 930, I don't think "cm_info" or "cm" are defined types (I only know this after reviewing someone else's code), just go with stdClass The change at line 225 is unnecessary The return type at line 283 should be "bool|completion_criteria_completion" and the fail state should be described above or in the return description $item and $grade could be described at 1275 and 1276 respectively I kept getting lost in the files because they have similar filenames. My apologies if my comments don't appear in the correct files. There were a few changes to things like indenting outside of the doc blocks. We should avoid these for now. They can be created as separate issues if you believe they are significant, but most of them were not.
        Hide
        moodle.com added a comment -

        Looks like I forgot to stop peer-reviewing this.

        Show
        moodle.com added a comment - Looks like I forgot to stop peer-reviewing this.
        Hide
        Ankit Agarwal added a comment -

        Thanks Michael for the feedback.
        I have made almost all the changes you suggested.
        Just to note afaik we are using classnames as datatypes where ever possible, and cm_info seems appropriate to me in lib/completionlib.php in the above mentioned cases
        Thanks

        Show
        Ankit Agarwal added a comment - Thanks Michael for the feedback. I have made almost all the changes you suggested. Just to note afaik we are using classnames as datatypes where ever possible, and cm_info seems appropriate to me in lib/completionlib.php in the above mentioned cases 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 -
        • completion_info::update_state(), completion_criteria_self::review() seem to be missing some param(s)
        • Some of the edited lines in the patch contains incorrect trailing whitespace, please review and fix.
        • No scope modifier specified for function "notify_changed" (lib/completion/data_object.php)
        • lib/completion/completion_criteria_duration.php (#188 - 2 spaces)

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - category completion seems incorrect. activity_completion is the one @ http://docs.moodle.org/dev/Core_APIs completion_info::update_state(), completion_criteria_self::review() seem to be missing some param(s) Some of the edited lines in the patch contains incorrect trailing whitespace, please review and fix. No scope modifier specified for function "notify_changed" (lib/completion/data_object.php) lib/completion/completion_criteria_duration.php (#188 - 2 spaces) Ciao
        Hide
        Rajesh Taneja added a comment -

        Thanks for pointing that Eloy,
        It should be completion and not activity_completion

        Fixing Core_APIs page

        Show
        Rajesh Taneja added a comment - Thanks for pointing that Eloy, It should be completion and not activity_completion Fixing Core_APIs page
        Hide
        Ankit Agarwal added a comment -

        Hi Eloy,
        I have made changes as per your suggestion.
        I guess I have documented all three variables of completion_info::update_state() already. Rest changes made.
        Also rebased
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Eloy, I have made changes as per your suggestion. I guess I have documented all three variables of completion_info::update_state() already. Rest changes made. Also rebased Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi, I'm reopening this related with the justification @ MDL-30996. I think we need to clearly separate and name each one of the APIs properly and right now, I'm not sure that is being achieved.

        Feel free to discuss it and get a final decision/amending of, as requested @ MDL-30996.

        Otherwise, it's looking great, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hi, I'm reopening this related with the justification @ MDL-30996 . I think we need to clearly separate and name each one of the APIs properly and right now, I'm not sure that is being achieved. Feel free to discuss it and get a final decision/amending of, as requested @ MDL-30996 . Otherwise, it's looking great, ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        As commented @ MDL-30996, we only must use the @access phpdocs tag for global-scope functions, not for class methods/members.

        So I'm reopening this to fix (delete) all those occurrences.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - As commented @ MDL-30996 , we only must use the @access phpdocs tag for global-scope functions, not for class methods/members. So I'm reopening this to fix (delete) all those occurrences. Ciao
        Hide
        Ankit Agarwal added a comment -

        updates made!
        Submitting for integration!
        Thanks

        Show
        Ankit Agarwal added a comment - updates made! Submitting for integration! Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Holding this open for 24h while Ankit fixes some minor remaining issues.

        Show
        Eloy Lafuente (stronk7) added a comment - Holding this open for 24h while Ankit fixes some minor remaining issues.
        Hide
        Ankit Agarwal added a comment -

        Changes made.
        Back to your court Eloy.
        Thanks

        Show
        Ankit Agarwal added a comment - Changes made. Back to your court Eloy. Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks!

        Note: I've added one extra commit about the remaining mistake reported by the phpdocs tester. After all there was a tiny mistake there. hard to see, really, but obvious once found.

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks! Note: I've added one extra commit about the remaining mistake reported by the phpdocs tester. After all there was a tiny mistake there. hard to see, really, but obvious once found.
        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 -

        FCT (fixed, closing, thanks). Ciao

        "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!"
        ~ Benjamin Disraeli

        Show
        Eloy Lafuente (stronk7) added a comment - FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli

          People

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

            Dates

            • Created:
              Updated:
              Resolved: