Projects Plugin: Add ShellCheck analyzer
ClosedPublic

Authored by gregormi on Sun, Dec 2, 3:49 PM.

Details

Summary

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:

Diff Detail

Repository
R40 Kate
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
gregormi created this revision.Sun, Dec 2, 3:49 PM
Restricted Application added a project: Kate. · View Herald TranscriptSun, Dec 2, 3:49 PM
Restricted Application added a subscriber: kwrite-devel. · View Herald Transcript
gregormi requested review of this revision.Sun, Dec 2, 3:49 PM
gregormi retitled this revision from Code Analyser: Add target programming language after the name of the analyser to Projects Plugin: Add ShellCheck analyser.Sun, Dec 2, 3:53 PM
gregormi edited the summary of this revision. (Show Details)
gregormi added a reviewer: Kate.
gregormi updated this revision to Diff 46718.Sun, Dec 2, 3:54 PM
gregormi edited the summary of this revision. (Show Details)
  • fix whitespace
kfunk added a subscriber: kfunk.Mon, Dec 3, 1:59 PM

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;

gregormi added inline comments.Mon, Dec 3, 8:47 PM
addons/project/kateprojectcodeanalysistool.h
112

Bought! Your suggestion is easier to understand while being more flexible.

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?
If you want to save time by avoiding creating the QRegularExpression every time parseLine() is called, you can make it static: static QRegularExpression regex(...);

gregormi updated this revision to Diff 47114.Sat, Dec 8, 2:05 PM
gregormi marked 2 inline comments as done.
  • Rebase on master and Use better method isSuccessfulExitCode
gregormi updated this revision to Diff 47118.Sat, Dec 8, 3:02 PM
  • Show number of processed files
  • Disable start button while analysis is running
  • Use QRegularExpression
gregormi retitled this revision from Projects Plugin: Add ShellCheck analyser to Projects Plugin: Add ShellCheck analyzer.Sat, Dec 8, 3:05 PM
gregormi edited the summary of this revision. (Show Details)
gregormi marked an inline comment as done.
gregormi updated this revision to Diff 47119.Sat, Dec 8, 3:22 PM
gregormi edited the summary of this revision. (Show Details)
  • Add short description for each tool and make first character of name upper case
gregormi edited the summary of this revision. (Show Details)Sat, Dec 8, 3:23 PM
ngraham added a subscriber: ngraham.Sat, Dec 8, 3:39 PM
ngraham added inline comments.
addons/project/tools/kateprojectcodeanalysistoolcppcheck.cpp
44

Does this need to be in quotes?

gregormi added inline comments.Sat, Dec 8, 3:46 PM
addons/project/tools/kateprojectcodeanalysistoolcppcheck.cpp
44

I was not sure, because I grabbed the exact text from the projects web site.

ngraham added inline comments.Sat, Dec 8, 3:48 PM
addons/project/tools/kateprojectcodeanalysistoolcppcheck.cpp
44

I think we can omit the quotation marks.

gregormi updated this revision to Diff 47122.Sat, Dec 8, 4:07 PM
gregormi edited the summary of this revision. (Show Details)
  • Remove quotes
gregormi marked 3 inline comments as done.Sat, Dec 8, 4:08 PM
dhaumann accepted this revision.Sat, Dec 8, 4:29 PM

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.

This revision is now accepted and ready to land.Sat, Dec 8, 4:29 PM
gregormi updated this revision to Diff 47125.Sat, Dec 8, 4:50 PM
  • const and i18np
gregormi marked 2 inline comments as done.Sat, Dec 8, 4:51 PM
This revision was automatically updated to reflect the committed changes.
rjvbb reopened this revision.Mon, Dec 10, 12:10 PM
rjvbb added a subscriber: rjvbb.

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
This revision is now accepted and ready to land.Mon, Dec 10, 12:10 PM

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 1

REPOSITORY

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

I can commit if you want (if nobody else beats me to it).