Review Request/New Framework: KF5::SyntaxHighlighting

[Scarlett Clark]

On Fri, Sep 30, 2016 at 10:32 AM, Volker Krause <vkrause at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160930/c1f0b9ce/attachment.html>