<p> Attention is currently required from: Sean Bright, Alec Davis. </p>
<p>Patch set 15:<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/+/18829">View Change</a></p><p>13 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">Commit Message:</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/+/18829/comment/9f72b48c_d65ae027">Patch Set #14, Line 20:</a> <code style="font-family:monospace,monospace"> same => n,Verbose(test_sentence=${REDIS(test_sentence)})</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The commit message is not the right place for an example. Include this in the XML documentation instead, and make sure to use the <example> tags.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/autoconfig.h.in:</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/+/18829/comment/d415db72_587ab72b">Patch Set #14, Line 29:</a> <code style="font-family:monospace,monospace">/* Define to 1 if using 'alloca.c'. */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Did you mean to remove CRAY_STACKSEG_END? Or is this a possible rebase issue?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/5589d2ec_d7304cd3">Patch Set #14, Line 504:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same for this, why is memory.h gone?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_hiredis.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/+/18829/comment/b96211ae_7cc39b68">Patch Set #14, Line 49:</a> <code style="font-family:monospace,monospace"> <syntax argsep=""></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why is the argsep empty? If it's not relevant, then remove the argsep attribute.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/1c4dad48_00688c2f">Patch Set #14, Line 50:</a> <code style="font-family:monospace,monospace"> <parameter name="valid redis syntax" required="true" /></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would use underscores instead of spaces, if only to adhere to how all other modules do it.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/5edf27b1_771ae18a">Patch Set #14, Line 53:</a> <code style="font-family:monospace,monospace"> <para>This function will pass without validation any Redis syntax to a Redis database</para></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">add a comma before and after "without validation"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/fba2ee85_4e2e21fc">Patch Set #14, Line 79:</a> <code style="font-family:monospace,monospace"> If you wish to find out if an entry exists, use a REDIS_EXISTS</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think "If you wish..." should be a separate <para> for readability.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/65ee2198_53993ed4">Patch Set #14, Line 121:</a> <code style="font-family:monospace,monospace"> <ref type="function">REDIS</ref></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I noticed you only have see also for the REDIS function for several of these functions.<br>Are the other functions not also possible relevant? If so, maybe include all the REDIS functions. That way, on a wiki page, a user can see all the other relevant REDIS functions available.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/3f85b012_e134f22b">Patch Set #14, Line 152:</a> <code style="font-family:monospace,monospace"> <synopsis>Background Save on module unload</synopsis></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The synopsis for this option is a little unclear and could be expanded/clarified</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/c3493e23_924e0da5">Patch Set #14, Line 211:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Extra line</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/3dbb0651_8488b393">Patch Set #14, Line 305:</a> <code style="font-family:monospace,monospace">static int __attribute__((format(printf, 3, 0))) hiredis_command(char* buf, size_t len, const char* format, ...) </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">redness</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/52abcc3b_1f80d1a6">Patch Set #14, Line 638:</a> <code style="font-family:monospace,monospace"> * even if live_dangerously is disabled.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I thought DB writing requires live dangerously be enabled. Should this not be consistent with that?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/4bc5c5e4_6502f671">Patch Set #14, Line 767:</a> <code style="font-family:monospace,monospace"> ast_debug(6, " password = %s\n", cfg->general->password);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think some debugs are fine but writing the password into the log on init might be a bit too much. I would at least remove that.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/18829">change 18829</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/+/18829"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ifdf4d33d2a2a5fd044fac13b201c7437de34ba6a </div>
<div style="display:none"> Gerrit-Change-Number: 18829 </div>
<div style="display:none"> Gerrit-PatchSet: 15 </div>
<div style="display:none"> Gerrit-Owner: Alec Davis <alec@bdt.co.nz> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </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: Alec Davis <alec@bdt.co.nz> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 29 Jul 2022 12:33:38 +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>