<p> Attention is currently required from: Sean Bright, Mark Murawski. </p>
<p>Patch set 8:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/17655">View Change</a></p><p>11 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">Patchset:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655?tab=comments">Patch Set #8:</a> </p><p style="white-space: pre-wrap; word-wrap: break-word;">Looks to be in pretty good shape, my complaints here are mostly cosmetic.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File funcs/func_groupcount.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/dc1dbcc2_fc9e965b">Patch Set #8, Line 87:</a> <code style="font-family:monospace,monospace"> <para>Inherit. Group membership will be kept when a channel is transferred.</para></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/df9b90f4_89c416ca">Patch Set #8, Line 115:</a> <code style="font-family:monospace,monospace"> <para>Example:</para></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Use proper <example> sections for examples, not a bunch of <para>s.<br>e.g. see https://gerrit.asterisk.org/c/asterisk/+/17586</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think that would also avoid having to surround variables in <literal>, because ${} expressions are otherwise parsed specially by the wiki.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/bce82eba_615568bb">Patch Set #8, Line 1003:</a> <code style="font-family:monospace,monospace"> .desc = "Gets or sets a channel group variable.\n\</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If that's the case, I would get rid of this and ensure that what exists here is all in the xmldocs.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/e06169b6_bd00cc58">Patch Set #8, Line 1171:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_WARNING, "GROUP_MATCH_LIST_NEXT() called after the last item was already asked for\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Indentation looks off here in this if block</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/61b8048b_3db262f7">Patch Set #8, Line 1267:</a> <code style="font-family:monospace,monospace"> .synopsis =</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same comments here about registering usage and xml docs for these two...</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File main/app.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/d0ef5e9c_c0f465cf">Patch Set #8, Line 2447:</a> <code style="font-family:monospace,monospace"> category = "";</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">needs to be indented</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/9ba123de_ce6347d6">Patch Set #8, Line 2517:</a> <code style="font-family:monospace,monospace"> return NULL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">indentation here and in the next if look off</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/255c53ae_eebdabfd">Patch Set #8, Line 2549:</a> <code style="font-family:monospace,monospace">{</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You're using spaces in this function for indentation, it needs to be tabs.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/841af85b_a004b05b">Patch Set #8, Line 2610:</a> <code style="font-family:monospace,monospace">{</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here: convert spaces to tabs - this whole blue block here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File main/cli.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/12c31650_afa1de4f">Patch Set #8, Line 1913:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Indentation here doesn't look quite right - appears to be 2 spaces as opposed to a tab.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/17655">change 17655</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/17655"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 18 </div>
<div style="display:none"> Gerrit-Change-Id: I23e48d1cdfc8adaffdfec2e936e56143603914f2 </div>
<div style="display:none"> Gerrit-Change-Number: 17655 </div>
<div style="display:none"> Gerrit-PatchSet: 8 </div>
<div style="display:none"> Gerrit-Owner: Mark Murawski <markm@intellasoft.net> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-Attention: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-Attention: Mark Murawski <markm@intellasoft.net> </div>
<div style="display:none"> Gerrit-Comment-Date: Sat, 05 Mar 2022 20:14:54 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>