Review Request/New Framework: KF5::SyntaxHighlighting

[Volker Krause]

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?

> 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 --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 173 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160930/d0d4c413/attachment.sig>