D17318: WIP: Code Analysis: Show file extentions where the tool runs on

Dominik Haumann noreply at phabricator.kde.org
Wed Dec 5 14:23:36 GMT 2018

dhaumann added a comment.

  I think this is fine. A minor comment from my side would be: I am not sure adding these kind of labels are alwas a good solution. I can see that it's useful to know which files are affected. Then again, once you know this, the label is just visual noise.


> kateprojectcodeanalysistool.h:62
> +     */
> +    virtual QString fileExtensions() = 0;
> +

Why not return a QStringList? The current version just forces parsing in other places. :)


Later: ok, now I see it's used in the regular expression. Still, I don't like how an implementation detail in a specific location (here: regex later) imposes design decisions here. And also in the UI, a label would rather be comma separated instead of | separated.

> kateprojectinfoviewcodeanalysis.cpp:105
> +    m_analysisTool = m_toolSelector->currentData(Qt::UserRole + 1).value<KateProjectCodeAnalysisTool*>();
> +    m_toolInfoLabel->setText(i18n("  The tool will be run on project files with these endings: %1.",
> +                                  m_analysisTool->fileExtensions()));

Why are there leading spaces in the label? Usually, spacing is up to the QStyle.

> kateprojectinfoviewcodeanalysis.h:66
>  private Q_SLOTS:
> +    void slotToolSelectionChanged(int);
> +

Everything in this file has comments. This function should have one as well.

  R40 Kate


To: gregormi, #kate
Cc: dhaumann, ngraham, kwrite-devel, hase, michaelh, demsking, cullmann, sars
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwrite-devel/attachments/20181205/421b586a/attachment.html>

More information about the KWrite-Devel mailing list