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:
    • Rank:
      37377

      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.

      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 Škoda 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 Škoda 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: