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

activity_completion 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 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.

        Gliffy Diagrams

          Activity

          Hide
          samhemelryk 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
          samhemelryk 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_frenz 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_frenz 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
          samhemelryk Sam Hemelryk added a comment -

          Rebased.

          Show
          samhemelryk Sam Hemelryk added a comment - Rebased.
          Hide
          salvetore 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
          salvetore 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 moodle.com added a comment -

          Looks like I forgot to stop peer-reviewing this.

          Show
          moodle.com moodle.com added a comment - Looks like I forgot to stop peer-reviewing this.
          Hide
          ankit_frenz 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_frenz 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
          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 -
          • 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
          stronk7 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
          rajeshtaneja Rajesh Taneja added a comment -

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

          Fixing Core_APIs page

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks for pointing that Eloy, It should be completion and not activity_completion Fixing Core_APIs page
          Hide
          ankit_frenz 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_frenz 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
          stronk7 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
          stronk7 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
          stronk7 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
          stronk7 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_frenz Ankit Agarwal added a comment -

          updates made!
          Submitting for integration!
          Thanks

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

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

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

          Changes made.
          Back to your court Eloy.
          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - Changes made. Back to your court Eloy. Thanks
          Hide
          stronk7 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
          stronk7 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
          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 -

          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
          stronk7 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:
                Fix Release Date:
                25/Jun/12