<p> Attention is currently required from: George Joseph. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/16121">View Change</a></p><p>1 comment:</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/+/16121?tab=comments">Patch Set #1:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'm kinda "iffy" on this review 😊 […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think that was quite the idea here, it was more simple things like this:</p><p style="white-space: pre-wrap; word-wrap: break-word;">same => n,If($["${feature-a}"="1"])<br>same => n,Gosub(dosomething,s,1)<br>same => n,Gosub(dosomethingelse,s,1)<br>same => n,Set(something=something)<br>same => n,NoOp(${DB_DELETE(something/thing)})<br>same => n,EndIf()<br>same => n,If($["${feature-b}"="1"])</p><p style="white-space: pre-wrap; word-wrap: break-word;">Currently, there are 3 ways this could be done, each of which is a bit hacky in some respect.</p><p style="white-space: pre-wrap; word-wrap: break-word;">same => n,GosubIf($["${feature-a}"="1"]?dosomething,s,1)<br>same => n,GosubIf($["${feature-a}"="1"]?dosomethingelse,s,1)<br>same => n,ExecIf($["${feature-a}"="1"]?Set(something=something)<br>same => n,GotoIf($["${feature-a}"!="1"]?postdbdel)<br>same => n,NoOp(${DB_DELETE(something/thing)}) ; functions always evaluate even if ExecIf is 0, so this is double trouble. This cannot be encapsulated in a conditional directly because it must never be parsed if false.<br>same => n(postdbdel),ExecIf($["{feature-b}"="1"]?) ... etc.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Problem: very repetitive. We shouldn't be checking the same condition more than once or twice. A better way:</p><p style="white-space: pre-wrap; word-wrap: break-word;">same => n,GotoIf($["${feature-a}"!="1"]?postfeaturea)<br>same => n(postfeaturea),GotoIf($["${feature-a}"!="1"]?postfeatureb)<br>same => n(postfeatureb),GotoIf($["${feature-b}"!="1"]?postfeaturec)</p><p style="white-space: pre-wrap; word-wrap: break-word;">Problem: a) The labels are confusing. The first one is structured differently from the last one, from the middle. You basically need a label for the end of each part, to skip over the block in the middle. Probably the cleanest way to do it now.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Here's a more semantic way to do it:</p><p style="white-space: pre-wrap; word-wrap: break-word;">same => n,While($["${feature-a}"="1"])<br>same => n,Gosub(dosomething,s,1)<br>same => n,Gosub(dosomethingelse,s,1)<br>same => n,Set(something=something)<br>same => n,NoOp(${DB_DELETE(something/thing)})<br>same => n,ExitWhile()<br>same => n,EndWhile()<br>same => n,While($["${feature-2}"="1"]) etc.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This is the most readable code, as it's very clear what's going on, doing what we want to do, instead of using a jillion GotoIfs in order to skip stuff we *don't* want to do. Problem: The ExitWhile is critical. Otherwise, we have an infinite loop, and it's easy to forget the ExitWhile. Thus:</p><p style="white-space: pre-wrap; word-wrap: break-word;">same => n,If($["${feature-a}"="1"])<br>same => n,Gosub(dosomething,s,1)<br>same => n,Gosub(dosomethingelse,s,1)<br>same => n,Set(something=something)<br>same => n,NoOp(${DB_DELETE(something/thing)})<br>same => n,EndIf()<br>same => n,If($["${feature-2}"="1"]) etc.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Basically, this is intended to be similar to the last case, a semantic way to execute a block just one time, so if there are a few of these, the dialplan is simplified, very easy to read and maintain and update. That was the main objective here, so Lua would not help for this case.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If you're still iffy, though, maybe we can toss this one ;)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/16121">change 16121</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/+/16121"/><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: I3aa3bd35a5add82465c6ee9bd86b64601f0e1f49 </div>
<div style="display:none"> Gerrit-Change-Number: 16121 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Attention: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 06 Jul 2021 13:49:28 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>