<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 30, 2016 at 10:32 AM, Volker Krause <span dir="ltr"><<a href="mailto:vkrause@kde.org" target="_blank">vkrause@kde.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks!<br>
<span class=""><br>
On Friday 30 September 2016 10:08:27 David Faure wrote:<br>
> On samedi 10 septembre 2016 17:47:00 CEST Volker Krause wrote:<br>
> > Hi,<br>
> ><br>
> > please review KF5::SyntaxHighlighting (syntax-highlighting in Git) for<br>
> > becoming a framework :)<br>
><br>
> Looks good. I found a few things though.<br>
><br>
> I see that a Jenkins job exists, but it's missing from this view<br>
> <a href="https://build.kde.org/view/Frameworks%20kf5-qt5/" rel="noreferrer" target="_blank">https://build.kde.org/view/<wbr>Frameworks%20kf5-qt5/</a><br>
<br>
</span>Is that something I can do directly, or is this done via a ticket for the CI?<br></blockquote><div><br></div><div>This is now fixed.</div><div>Cheers,</div><div>Scarlett</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> Fully missing API docs in :<br>
>   DefinitionDownloader<br>
>   HtmlHighlighter<br>
<br>
</span>Both are not installed, they are only exported for the CLI tool.<br>
<span class=""><br>
> Also seen in HtmlHighlighter:<br>
>     void setOutputFile(FILE *fileHandle);<br>
> Shouldn't this use QFile or QIODevice instead?<br>
> Or QTextStream, looking at the implementation.<br>
<br>
</span>Possible, the reason for this was supporting output to stdout in the CLI tool.<br>
But as mentioned above, this is not installed/public API anway.<br>
<span class=""><br>
> I'm also concerned that this class has direct member vars rather than a d<br>
> pointer.<br>
><br>
> SyntaxHighligher: missing API docs on methods; missing d pointer.<br>
<br>
</span>The d pointer is in the base class already, so I don't think we need another<br>
one here, do we?<br>
<span class=""><br>
> Once these issues are solved you can set the release flag to true in the<br>
> yaml file, from my point of view.<br>
<br>
</span>Regards,<br>
Volker</blockquote></div><br></div></div>