[Asterisk-code-review] func_groupcount.c: Adding Group Variables and additional Group functions (asterisk[18])

[N A]

Attention is currently required from: Sean Bright, Mark Murawski.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/17655 )

Change subject: func_groupcount.c: Adding Group Variables and additional Group functions
......................................................................


Patch Set 8: Code-Review-1

(11 comments)

Patchset:

PS8: 
Looks to be in pretty good shape, my complaints here are mostly cosmetic.


File funcs/func_groupcount.c:

https://gerrit.asterisk.org/c/asterisk/+/17655/comment/dc1dbcc2_fc9e965b 
PS8, Line 87: 						<para>Inherit. Group membership will be kept when a channel is transferred.</para>
I think more clarification and explanation about the inherit option is needed. What kinds of transfers are we talking about here? What transfer scenarios would require this and which would not?


https://gerrit.asterisk.org/c/asterisk/+/17655/comment/df9b90f4_89c416ca 
PS8, Line 115:                         <para>Example:</para>
Use proper <example> sections for examples, not a bunch of <para>s.
e.g. see https://gerrit.asterisk.org/c/asterisk/+/17586

I think that would also avoid having to surround variables in <literal>, because ${} expressions are otherwise parsed specially by the wiki.


https://gerrit.asterisk.org/c/asterisk/+/17655/comment/bce82eba_615568bb 
PS8, Line 1003: 	.desc = "Gets or sets a channel group variable.\n\
I think .desc is reminiscent of the way things used to be, perhaps before the XML documentation existed. I don't see very much left in the tree that still registers its usage.

If that's the case, I would get rid of this and ensure that what exists here is all in the xmldocs.


https://gerrit.asterisk.org/c/asterisk/+/17655/comment/e06169b6_bd00cc58 
PS8, Line 1171: 	  ast_log(LOG_WARNING, "GROUP_MATCH_LIST_NEXT() called after the last item was already asked for\n");
Indentation looks off here in this if block


https://gerrit.asterisk.org/c/asterisk/+/17655/comment/61b8048b_3db262f7 
PS8, Line 1267: 	.synopsis =
Same comments here about registering usage and xml docs for these two...


File main/app.c:

https://gerrit.asterisk.org/c/asterisk/+/17655/comment/d0ef5e9c_c0f465cf 
PS8, Line 2447:         category = "";
needs to be indented


https://gerrit.asterisk.org/c/asterisk/+/17655/comment/9ba123de_ce6347d6 
PS8, Line 2517:           return NULL;
indentation here and in the next if look off


https://gerrit.asterisk.org/c/asterisk/+/17655/comment/255c53ae_eebdabfd 
PS8, Line 2549: {
You're using spaces in this function for indentation, it needs to be tabs.


https://gerrit.asterisk.org/c/asterisk/+/17655/comment/841af85b_a004b05b 
PS8, Line 2610: {
Same here: convert spaces to tabs - this whole blue block here.


File main/cli.c:

https://gerrit.asterisk.org/c/asterisk/+/17655/comment/12c31650_afa1de4f 
PS8, Line 1913: 
Indentation here doesn't look quite right - appears to be 2 spaces as opposed to a tab.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/17655
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: I23e48d1cdfc8adaffdfec2e936e56143603914f2
Gerrit-Change-Number: 17655
Gerrit-PatchSet: 8
Gerrit-Owner: Mark Murawski <markm at intellasoft.net>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Mark Murawski <markm at intellasoft.net>
Gerrit-Comment-Date: Sat, 05 Mar 2022 20:14:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220305/786c2736/attachment.html>