dhaumann added subscribers: cullmann, vkrause, dhaumann. dhaumann requested changes to this revision. dhaumann added a comment. This revision now requires changes to proceed.
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? 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? Would it also be a solution for you to simply contribute your highlighting files? Please explain :-) 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. 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. So I propose to change the naming: - void addCustomSearchPath(const QString &); - QVector<QString> customSearchPaths() const; - QVector<QString> m_customSearchPaths; In addition, I would welcome reviews from @vkrause and @cullmann. REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D7699 To: zrax, #kate, #framework_syntax_hightlighting, dhaumann Cc: dhaumann, vkrause, cullmann, #framework_syntax_hightlighting, #frameworks
