<table><tr><td style="">dhaumann added subscribers: cullmann, vkrause, dhaumann.<br />dhaumann requested changes to this revision.<br />dhaumann 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/D7699" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>First of all, could you provide more details about why you need this functionality? Is it in some other KDE app, or some app outside of the KDE infrastructure? What's the use case exactly?<br />
So far I have the impression this is used outside of KDE, and you intentionally want to ship your own stuff only. Is that the case?<br />
Would it also be a solution for you to simply contribute your highlighting files?</p>

<p>Please explain :-)</p>

<p>Besides that, we can add this, I don't have strong objections so far. In general, the patch is ok, but a unit test is missing.</p>

<p>General rule of thumb: If you add a setter (in this case addSearchPath()), you also need to add a getter (QStringList customSearchPaths() or so). Why? Because otherwise this code is not unit testable: Without the getter, you can write a test and add a search path. How do you now check this search path was really added? Impossible. So please *always* make sure your patches are testable, best by directly adding unit tests.</p>

<p>So I propose to change the naming:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">void addCustomSearchPath(const QString &);</li>
<li class="remarkup-list-item">QVector<QString> customSearchPaths() const;</li>
<li class="remarkup-list-item">QVector<QString> m_customSearchPaths;</li>
</ul>

<p>In addition, I would welcome reviews from <a href="https://phabricator.kde.org/p/vkrause/" style="
              border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;" rel="noreferrer">@vkrause</a> and <a href="https://phabricator.kde.org/p/cullmann/" style="
              border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;" rel="noreferrer">@cullmann</a>.</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R216 Syntax Highlighting</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D7699" rel="noreferrer">https://phabricator.kde.org/D7699</a></div></div><br /><div><strong>To: </strong>zrax, Kate, Framework: Syntax Hightlighting, dhaumann<br /><strong>Cc: </strong>dhaumann, vkrause, cullmann, Framework: Syntax Hightlighting, Frameworks<br /></div>