<table><tr><td style="">dvratil requested changes to this revision.<br />dvratil added a comment.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D23979">View Revision</a></tr></table><br /><div><div><p>Nice, thanks for the patch.</p>
<p>I understand the desire to not sent unnecessary query parameters when they do not differ from the default value, but the way the code stands now it look like "magic" to me and is error prone - how about defining default value constants like e.g.</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">namespace {
static constexpr bool useDomainAdminAccessDefault = false;
}</pre></div>
<p>and then all the conditions would look like</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">if (d->useDomainAdminAccess != useDomainAdminAccessDefault) {
...
}</pre></div>
<p>Which I think is not only much easier to understand but also prevents mistakes like those below and makes changing the default super-easy (single place in the entire file).</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D23979#inline-135762">View Inline</a><span style="color: #4b4d51; font-weight: bold;">permissioncreatejob.cpp:84</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">useDomainAdminAccess</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">query</span><span class="p">.</span><span class="n">addQueryItem</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"useDomainAdminAccess"</span><span class="p">),</span> <span class="n">Utils</span><span style="color: #aa2211">::</span><span class="n">bool2Str</span><span class="p">(</span><span class="n">useDomainAdminAccess</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This property defaults to false, so shouldn't the condition be the other way around, so it gets sent when it's different from the default?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D23979#inline-135761">View Inline</a><span style="color: #4b4d51; font-weight: bold;">permissiondeletejob.cpp:132</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">withDriveSupportQ</span>uery</span><span class="p">.</span><span class="n">addQueryItem</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"supportsAllDrives"</span><span class="p">),</span> <span class="n">Utils</span><span style="color: #aa2211">::</span><span class="n">bool2Str</span><span class="p">(</span><span class="n">d</span><span style="color: #aa2211">-></span><span class="n">supportsAllDrives</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">url</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">setQuery</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">withDriveSupportQuery</span></span><span class="bright"></span><span class="p"><span class="bright">);</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span class="n"><span class="bright">q</span>uery</span><span class="p">.</span><span class="n">addQueryItem</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"supportsAllDrives"</span><span class="p">),</span> <span class="n">Utils</span><span style="color: #aa2211">::</span><span class="n">bool2Str</span><span class="p">(</span><span class="n">d</span><span style="color: #aa2211">-></span><span class="n">supportsAllDrives</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">if</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">!</span></span><span class="bright"></span><span class="n"><span class="bright">d</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">useDomainAdminAccess</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">query</span><span class="p">.</span><span class="n">addQueryItem</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"useDomainAdminAccess"</span><span class="p">),</span> <span class="n">Utils</span><span style="color: #aa2211">::</span><span class="n">bool2Str</span><span class="p">(</span><span class="n">d</span><span style="color: #aa2211">-></span><span class="n">useDomainAdminAccess</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This property defaults to false, so shouldn't the condition be the other way around, so it gets sent when it's different from the default?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D23979#inline-135760">View Inline</a><span style="color: #4b4d51; font-weight: bold;">permissionmodifyjob.cpp:77</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">removeExpiration</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">query</span><span class="p">.</span><span class="n">addQueryItem</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"removeExpiration"</span><span class="p">),</span> <span class="n">Utils</span><span style="color: #aa2211">::</span><span class="n">bool2Str</span><span class="p">(</span><span class="n">removeExpiration</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This property defaults to <tt style="background: #ebebeb; font-size: 13px;">false</tt>, so shouldn't the condition be the other way around, so it gets sent when it's different from the default? Same below...</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R477 KGAPI Library</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D23979">https://phabricator.kde.org/D23979</a></div></div><br /><div><strong>To: </strong>barchiesi, dvratil<br /><strong>Cc: </strong>LibKGAPI, kde-pim, barchiesi, fbampaloukas, dvasin, rodsevich, winterz, vkrause, mlaurent, knauss, dvratil<br /></div>