Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.2.1
    • Fix Version/s: 2.3
    • Component/s: Documentation, Files API
    • Labels:

      Description

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

        1. 30973.xml
          8 kB
          Eloy Lafuente (stronk7)

          Activity

          Hide
          Rajesh Taneja added a comment -

          Hello Dongsheng,

          You might want to consider fixing following:

          1. category is file and not files
          2. package should be core_file and not core (full frankenstyle)
          3. It will be nice if you can put description for params (@param string $component)
          4. @var is one-liner
          5. @subpackage used on 2421.
          6. IMO, no need for category in curl_cache as we don't want to show this to plugin creator.

          Rest looks Good

          Show
          Rajesh Taneja added a comment - Hello Dongsheng, You might want to consider fixing following: category is file and not files package should be core_file and not core (full frankenstyle) It will be nice if you can put description for params (@param string $component) @var is one-liner @subpackage used on 2421. IMO, no need for category in curl_cache as we don't want to show this to plugin creator. Rest looks Good
          Hide
          Dongsheng Cai added a comment -

          Sorry the 2.2 branch is not update, I fixed a few problems spotted by Rajesh

          • use one-liner for curl and curl_cache class
          • removed @category from curl_cache
          • keep in-line comments, added todos in DocBlock
          Show
          Dongsheng Cai added a comment - Sorry the 2.2 branch is not update, I fixed a few problems spotted by Rajesh use one-liner for curl and curl_cache class removed @category from curl_cache keep in-line comments, added todos in DocBlock
          Hide
          Martin Dougiamas added a comment - - edited

          @package is actually core_files (see get_core_subsystems() )

          @category files as well. I think it may be in too many places. It just needs to be on the API functions that plugins would use (and the implementations of callbacks as you did it for mod/xxx)

          Can you please look at http://docs.moodle.org/dev/File_API to make sure it's up to date?

          Show
          Martin Dougiamas added a comment - - edited @package is actually core_files (see get_core_subsystems() ) @category files as well. I think it may be in too many places. It just needs to be on the API functions that plugins would use (and the implementations of callbacks as you did it for mod/xxx) Can you please look at http://docs.moodle.org/dev/File_API to make sure it's up to date?
          Hide
          Dongsheng Cai added a comment -

          Removed @category from file level docblock, added @category to public file apis.

          Show
          Dongsheng Cai added a comment - Removed @category from file level docblock, added @category to public file apis.
          Hide
          Petr Skoda added a comment -

          "core_files" was the stuff in /files/ directory only - we needed it for JS module there; the file API itself was "file" or "core_file"

          my +1 to revert it back to core_file

          Show
          Petr Skoda added a comment - "core_files" was the stuff in /files/ directory only - we needed it for JS module there; the file API itself was "file" or "core_file" my +1 to revert it back to core_file
          Hide
          Michael de Raadt added a comment - - edited

          Wow! That's a lot of files.

          I noted that you have not added descriptions to function parameters. This needs to happen. We want to help developers know how to call these functions. Returns can can also benefit from descriptions, but this is often obvious after the function description.

          There needs to be a blank comment line after a function description and before the parameters (or other tags). This is needed in:

          • lib/filebrowser/file_browser.php
          • lib/filebrowser/file_info.php
          • lib/filebrowser/file_info_context_user.php (get_parent())
          • lib/filebrowser/file_info_stored.php
          • lib/filebrowser/virtual_root_file.php
          • lib/filelib.php
          • lib/filestorage/file_storage.php (move_area_files_to_new_context())
          • lib/filestorage/zip_archive.php
          • lib/filestorage/zip_packer.php

          In lib/filebrowser/file_info_context_course.php:

          • At Lines 331 and 340, the descriptions are questions. Instead they should describe what the function does (otherwise this could be confusing, particularly for non-native English speakers). This happens again at 529, 548, 638 and 657.
          • The @see needs to be corrected at line 474.
            Continuing...

          In lib/filelib.php, changes like the one at line 353 should probably be avoided as part of this task. They could be handled in a separate issue.

          There were some unnecessary blank comment lines within the tag lines (after @category tags).

          • lib/filelib.php
          • mod/assignment/lib.php
          • lib/questionlib.php
          • mod/data/lib.php
          • ...all the files affected in /mod

          The files in /mod really need parameter descriptions.

          Apart from these minor points, your docblock comments are consistently technically correct, more so than others I have seen so far.

          Show
          Michael de Raadt added a comment - - edited Wow! That's a lot of files. I noted that you have not added descriptions to function parameters. This needs to happen. We want to help developers know how to call these functions. Returns can can also benefit from descriptions, but this is often obvious after the function description. There needs to be a blank comment line after a function description and before the parameters (or other tags). This is needed in: lib/filebrowser/file_browser.php lib/filebrowser/file_info.php lib/filebrowser/file_info_context_user.php ( get_parent() ) lib/filebrowser/file_info_stored.php lib/filebrowser/virtual_root_file.php lib/filelib.php lib/filestorage/file_storage.php ( move_area_files_to_new_context() ) lib/filestorage/zip_archive.php lib/filestorage/zip_packer.php In lib/filebrowser/file_info_context_course.php: At Lines 331 and 340, the descriptions are questions. Instead they should describe what the function does (otherwise this could be confusing, particularly for non-native English speakers). This happens again at 529, 548, 638 and 657. The @see needs to be corrected at line 474. Continuing... In lib/filelib.php, changes like the one at line 353 should probably be avoided as part of this task. They could be handled in a separate issue. There were some unnecessary blank comment lines within the tag lines (after @category tags). lib/filelib.php mod/assignment/lib.php lib/questionlib.php mod/data/lib.php ...all the files affected in /mod The files in /mod really need parameter descriptions. Apart from these minor points, your docblock comments are consistently technically correct, more so than others I have seen so far.
          Hide
          Dongsheng Cai added a comment -

          Michael, I fixed the problems you commented.

          Show
          Dongsheng Cai added a comment - Michael, I fixed the problems you commented.
          Hide
          Michael de Raadt added a comment -

          Sending this to integration, then...

          Show
          Michael de Raadt added a comment - Sending this to integration, then...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          wrong repository url

          Show
          Eloy Lafuente (stronk7) added a comment - wrong repository url
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Dong, I'm attaching one xml file (30973.xml) showing some problems related with the phpdocs added by the patch, mainly some missing params here and there... hope it helps, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Dong, I'm attaching one xml file (30973.xml) showing some problems related with the phpdocs added by the patch, mainly some missing params here and there... hope it helps, ciao
          Hide
          Dongsheng Cai added a comment -

          Fixed a few phpdocs issues

          Show
          Dongsheng Cai added a comment - Fixed a few phpdocs issues
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Nice one, Dong, just passed the checker again and found these:

          • filelib, line 1521: you've added the "@category files" in the middle, breaking the @todo tag contents.
          • don't use the @access tag in OOP methods. It should be only used in global-scope procedural functions.
          • Don't use the @see tag inline. It's not supported. Replace all occurrences of "{@see" by "{@link".
          • lib/filestorage/zip_archive.php, line 268: "Line indented incorrectly; expected at least 12 spaces, found 8"

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Nice one, Dong, just passed the checker again and found these: filelib, line 1521: you've added the "@category files" in the middle, breaking the @todo tag contents. don't use the @access tag in OOP methods. It should be only used in global-scope procedural functions. Don't use the @see tag inline. It's not supported. Replace all occurrences of "{@see" by "{@link". lib/filestorage/zip_archive.php, line 268: "Line indented incorrectly; expected at least 12 spaces, found 8" Ciao
          Hide
          Dongsheng Cai added a comment -

          Thanks Eloy

          just fixed issues you commented.

          Show
          Dongsheng Cai added a comment - Thanks Eloy just fixed issues you commented.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been integrated after adding one extra commits that fixes some incorrect @package tags along the whole workshopform subplugin.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated after adding one extra commits that fixes some incorrect @package tags along the whole workshopform subplugin. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Nobody tested this. Passing.

          Show
          Eloy Lafuente (stronk7) added a comment - Nobody tested this. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well,

          I wish I said it every time
          you do the things you do.
          You always lend a helping hand,
          and I'm filled with gratitude.

          You are strong and generous
          for each and everyone one of us.
          I am eternally grateful,
          I cannot say thanks enough.

          Sorry for the (un)cool bit above, lol. Closing this as fixed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, I wish I said it every time you do the things you do. You always lend a helping hand, and I'm filled with gratitude. You are strong and generous for each and everyone one of us. I am eternally grateful, I cannot say thanks enough. Sorry for the (un)cool bit above, lol. Closing this as fixed. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: