[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:

(4 comments)

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

Line 162:                            const char *like);
> This is actually at the correct indention. It's not part of the callback, i
Ah, I see now. Cool beans. Thanks for the clarification.


Line 167:  * \param like
> I'm unsure. It's not specified in the original function. Otherwise, I would
This comment may be pointless now if you end up refactoring the code to remove the parameter :)


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);
> This function is essentially an exact replica of ast_update_module_list. Th
Ah, okay. I contend that we may want to discuss w/ Matt because it *feels* like (and I may be wrong) this is actually fetching a list. I did not see anything being updated (other than the list that was being created for the fetch process).


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

Line 158: const char *like
> Like may not be doing anything here, but if someone else wants to use it in
There is a concept in Software Engineering dubbed, YAGNI, or You Ain't Gonna Need It. The thought is that anything added when it's just foreseen that it will be needed and not presently needed, adds weight to the system. You want to try and keep your APIs as simple as possible.

So, my thoughts are if someone needs it later, they can extend or modify (preferably extend) your existing implementation. And, since there are zero uses of it now it should be removed :)


-- 
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: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes