Differential D17314

Projects Plugin: Add ShellCheck analyzer
ClosedPublic
Actions

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

Details

Reviewers
dhaumann
Group Reviewers
Kate
Commits
R40:407928e4693e: Projects Plugin: Add ShellCheck analyzer
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
Branch
arcpatch-D17314
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5825
Build 5843: arc lint + arc unit
gregormi created this revision.Dec 2 2018, 3:49 PM
Restricted Application added a project: Kate. · View Herald TranscriptDec 2 2018, 3:49 PM
Restricted Application added a subscriber: kwrite-devel. · View Herald Transcript
gregormi requested review of this revision.Dec 2 2018, 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.Dec 2 2018, 3:53 PM
gregormi edited the summary of this revision. (Show Details)
gregormi added a reviewer: Kate.
gregormi updated this revision to Diff 46718.Dec 2 2018, 3:54 PM
gregormi edited the summary of this revision. (Show Details)
  • fix whitespace
kfunk added a subscriber: kfunk.Dec 3 2018, 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.Dec 3 2018, 8:47 PM
addons/project/kateprojectcodeanalysistool.h
112

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

dhaumann added a subscriber: dhaumann.Dec 3 2018, 9:38 PM

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.Dec 8 2018, 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.Dec 8 2018, 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.Dec 8 2018, 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.Dec 8 2018, 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)Dec 8 2018, 3:23 PM
ngraham added a subscriber: ngraham.Dec 8 2018, 3:39 PM
ngraham added inline comments.
addons/project/tools/kateprojectcodeanalysistoolcppcheck.cpp
44

Does this need to be in quotes?

gregormi added inline comments.Dec 8 2018, 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.Dec 8 2018, 3:48 PM
addons/project/tools/kateprojectcodeanalysistoolcppcheck.cpp
44

I think we can omit the quotation marks.

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

Looks good to me, but please consider using i18np to enable correct plural translations.

addons/project/kateprojectcodeanalysistool.h
60 ↗(On Diff #47122)

make the function const (can be done in a separate patch that also makes name() etc const.

addons/project/kateprojectinfoviewcodeanalysis.cpp
246 ↗(On Diff #47122)

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.Dec 8 2018, 4:29 PM
gregormi updated this revision to Diff 47125.Dec 8 2018, 4:50 PM
  • const and i18np
gregormi marked 2 inline comments as done.Dec 8 2018, 4:51 PM
Closed by commit R40:407928e4693e: Projects Plugin: Add ShellCheck analyzer (authored by gregormi). · Explain WhyDec 8 2018, 4:54 PM
This revision was automatically updated to reflect the committed changes.
rjvbb reopened this revision.Dec 10 2018, 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 ↗(On Diff #47122)
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.Dec 10 2018, 12:10 PM
gregormi added a comment.Dec 10 2018, 12:21 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

rjvbb added a comment.Dec 10 2018, 12:29 PM

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

Showing Only Differences

This revision modifies 5 more files that are hidden because they were not modified between selected diffs and they have no inline comments.

Revision Contents
Changeset List

Diff 47125

View Options

addons/project/autotests/test1.cpp

Loading...
View Options

addons/project/kateprojectcodeanalysistool.h

Loading...
View Options

addons/project/kateprojectinfoviewcodeanalysis.cpp

Loading...
View Options

addons/project/tools/kateprojectcodeanalysistoolcppcheck.h

Loading...
View Options

addons/project/tools/kateprojectcodeanalysistoolcppcheck.cpp

Loading...
View Options

addons/project/tools/kateprojectcodeanalysistoolflake8.h

Loading...
View Options

addons/project/tools/kateprojectcodeanalysistoolflake8.cpp

Loading...
View Options

addons/project/tools/kateprojectcodeanalysistoolshellcheck.h

Loading...
View Options

addons/project/tools/kateprojectcodeanalysistoolshellcheck.cpp

Loading...