Plus:
- Add target programming language after the name of each analyzer.
- Add number of processed files
- Disable Start button as long as analysis is running
Screenshot of selection box:
Screenshot of a finished run:
Tool description:
Details
Details
- Reviewers
dhaumann - Group Reviewers
Kate - Commits
- R40:407928e4693e: Projects Plugin: Add ShellCheck analyzer
Diff Detail
Diff Detail
- Repository
- R40 Kate
- Branch
- arcpatch-D17314
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 5825 Build 5843: arc lint + arc unit
Comment Actions
Just my 2 cent when I saw this review, didn't look further :)
| addons/project/kateprojectcodeanalysistool.h | ||
|---|---|---|
| 112 | This looks overly complicated and limited for an interface to me. Why not something like bool isSuccessfulRC(int rc) const? Default impl: return rc == 0; | |
| addons/project/kateprojectcodeanalysistool.h | ||
|---|---|---|
| 112 | Bought! Your suggestion is easier to understand while being more flexible. | |
Comment Actions
I think Kevin's suggestion is a good one. One more iteration?
| addons/project/tools/kateprojectcodeanalysistoolshellcheck.cpp | ||
|---|---|---|
| 89 | QRegExp is deprecated in favor of QRegeularExplression. Could you replace this? | |
Comment Actions
- Show number of processed files
- Disable start button while analysis is running
- Use QRegularExpression
| addons/project/tools/kateprojectcodeanalysistoolcppcheck.cpp | ||
|---|---|---|
| 44 | Does this need to be in quotes? | |
| addons/project/tools/kateprojectcodeanalysistoolcppcheck.cpp | ||
|---|---|---|
| 44 | I was not sure, because I grabbed the exact text from the projects web site. | |
| addons/project/tools/kateprojectcodeanalysistoolcppcheck.cpp | ||
|---|---|---|
| 44 | I think we can omit the quotation marks. | |
Comment Actions
Looks good to me, but please consider using i18np to enable correct plural translations.
| addons/project/kateprojectcodeanalysistool.h | ||
|---|---|---|
| 60 | make the function const (can be done in a separate patch that also makes name() etc const. | |
| addons/project/kateprojectinfoviewcodeanalysis.cpp | ||
| 246 | Instead of writing file(s) you could also use the i18np form to get a better translation: https://techbase.kde.org/Development/Tutorials/Localization/i18n#Plurals Think of languages where it's not possible to simply add a "(s)" to imply a correct plural form. Even in German it already starts to look awkward: "Analyse für 13 Datai(en) beendet" or so... Same below. | |
Comment Actions
I'm not finding any information on a QStringLiteral header file in the official documentation; it certainly doesn't exist in Qt 5.9.7 yet. AFAIK the header to include is the one for QString.
| addons/project/autotests/test1.cpp | ||
|---|---|---|
| 27 | addons/project/autotests/test1.cpp:27:10: fatal error: 'QStringLiteral' file not found :info:build #include <QStringLiteral> :info:build ^ :info:build 1 error generated. :info:build make[2]: *** [addons/project/autotests/CMakeFiles/projectplugin_test.dir/test1.cpp.o] Error 1 | |
Comment Actions
Yes, current master does not compile because of that. Sorry. #include
<QString> works. I cannot commit ATM.
Gregor
Am 10.12.2018 13:10 schrieb René J.V. Bertin:
View Revision
rjvbb reopened this revision.
rjvbb added a comment.
This revision is now accepted and ready to land.I'm not finding any information on a QStringLiteral header file in the official documentation; it certainly doesn't exist in Qt 5.9.7 yet. AFAIK the header to include is the one for QString.
INLINE COMMENTS
View Inline [1]test1.cpp:27
#include <QStringLiteral>
addons/project/autotests/test1.cpp:27:10: fatal error: 'QStringLiteral' file not found
:info:build #include <QStringLiteral>
:info:build ^
:info:build 1 error generated.
:info:build make[2]: *** [addons/project/autotests/CMakeFiles/projectplugin_test.dir/test1.cpp.o] Error 1REPOSITORY
R40 Kate
REVISION DETAIL
https://phabricator.kde.org/D17314 [2]TO: gregormi, Kate, dhaumann
CC: rjvbb, ngraham, dhaumann, kfunk, kwrite-devel, hase, michaelh, demsking, cullmann, sars
Links:
[1] https://phabricator.kde.org/D17314#inline-95941
[2] https://phabricator.kde.org/D17314