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