[Asterisk-code-review] ARI: Added new functionality to get all module information. (asterisk[master])

[Ashley Sanders]

Ashley Sanders has posted comments on this change.

Change subject: ARI: Added new functionality to get all module information.
......................................................................


Patch Set 2: Code-Review-1

(9 comments)

https://gerrit.asterisk.org/#/c/747/2/CHANGES
File CHANGES:

Line 194:  * A new feature has been added that enables the retrieval of modules and
        :    module information through a HTTP request.
change: [...] through a HTTP request.
to:  [...] through an HTTP request.


https://gerrit.asterisk.org/#/c/747/2/include/asterisk/module.h
File include/asterisk/module.h:

Line 162:                            const char *like);
Since you added the \since tag on line 157, maybe fix this indentation while you are here :)


Line 166:  * \param modentry A callback to an updated function
Based on the documentation for ast_update_module_list, I think this is supposed to read:

 * \param modentry A callback to an updater function.


Line 167:  * \param like
What does this parameter do?


Line 175: int ast_update_module_list_data(int (*modentry)(const char *module, const char *description,
        :                                                 int usecnt, const char *status, const char *like,
        :                                                 enum ast_module_support_level support_level,
        :                                                 void *data), const char *like, void *data);
Does this function really update the module list data or does it just fetch the module list, with extra data?
If it's only doing the latter, perhaps consider renaming it to ast_fetch_module_list or ast_fetch_module_list_data.


https://gerrit.asterisk.org/#/c/747/2/res/ari/resource_asterisk.c
File res/ari/resource_asterisk.c:

Line 146:  * \param module Resource name
        :  * \param description
        :  * \param usecnt Resource use count
        :  * \param status
        :  * \param like
        :  * \param support_level
        :  * \param module_data_list Resource array
I think you should go ahead and add a small blurb next to each of these parameters to describe their purpose. Nothing too fancy - just follow the convention you already used for module, usercnt, and module_data_list.


Line 154:  * \retval 0 if no resource
if no resource exists


Line 158: const char *like
'like' is not doing anything and is set as NULL when ast_update_module_list is invoked on line 183.

So... I think you can safely remove this parameter. Since this is the callback for ast_update_module_list_data, you will also need to update that function and it's prototype, and where that function is invoked on line 183 below.


Line 176: headers
Is this parameter required as a parameter because of a contract? If it is not, you can remove it. It's not being used at all.


-- 
To view, visit https://gerrit.asterisk.org/747
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I63cbbf0ec0c3544cc45ed2a588dceabe91c5e0b0
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes