Port kdev_format_source script to C++/Qt
ClosedPublic

Authored by antonanikin on Oct 4 2016, 3:14 AM.

Diff Detail

Repository
R33 KDevPlatform
Lint
Lint Skipped
Unit
Unit Tests Skipped
antonanikin updated this revision to Diff 7070.Oct 4 2016, 3:14 AM
antonanikin retitled this revision from to Port kdev_format_source script to C++/Qt.
antonanikin updated this object.
antonanikin edited the test plan for this revision. (Show Details)
antonanikin added a reviewer: KDevelop.
antonanikin set the repository for this revision to R33 KDevPlatform.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptOct 4 2016, 3:14 AM
antonanikin planned changes to this revision.Oct 4 2016, 3:21 AM
antonanikin updated this revision to Diff 7071.Oct 4 2016, 3:44 AM

sorry for previous (incorrect) version.

apol added a subscriber: apol.Oct 4 2016, 10:56 AM

A bit of background? Why are you doing this? (I've never used this script myself ^^')

antonanikin added a comment.EditedOct 4 2016, 11:02 AM

A bit of background? Why are you doing this?

Kevin Funk open task for this patch. It seems to be useful for windows-porting.

(I've never used this script myself ^^')

But I use it together with uncrustify tool (enable appropriate KDevelop setting for Source Formatter) :)

kfunk requested changes to this revision.Oct 12 2016, 7:59 PM
kfunk added a reviewer: kfunk.
kfunk added a subscriber: kfunk.

Please add a unit test which calls kdev_format_source on a few (simple) format files.

util/kdev_format_source.cpp
52 ↗(On Diff #7071)

Factor out the detection of the format file, e.g. findFormatFile(...). This also reduces the indent of the following lines.

58 ↗(On Diff #7071)

Style: CamelCase

64 ↗(On Diff #7071)

I'd try to separate the steps a) parsing the format file + b) executing the commands in the format file.

For parsing, something simple as QVector<FormatLine> parseFormatFile(), where FormatLine is a struct { QVector<QString> wildcards; QString command; }. Then use this function.

108 ↗(On Diff #7071)

Something's fishy here. This is not in the correct scope is it? If I understand correctly, then there can only be one call to QProcess::execute(command) per run? This can't be correct as there may be multiple format lines in a format file; each of them executing a different command. Please check.

This revision now requires changes to proceed.Oct 12 2016, 7:59 PM
kfunk added a comment.Oct 12 2016, 8:00 PM

Generally, thanks a lot for working on this task! Much appreciated!

antonanikin updated this revision to Diff 7414.Oct 16 2016, 8:57 AM
antonanikin edited edge metadata.
antonanikin marked 4 inline comments as done.

Something's fishy here. This is not in the correct scope is it? If I understand correctly, then there can only be one call to QProcess::execute(command) per run? This can't be correct as there may be multiple format lines in a format file; each of them executing a different command. Please check.

@kfunk, we should execute only one command per run - it's the original script behavior. It has simple sense - if we found some match we should apply only this command because we have "specialized" wildcards for some custom cases (which placed at the beginning of format file) and (usually) one "global" command ("*" wildcard) for all other cases.

kfunk requested changes to this revision.Oct 17 2016, 6:50 AM
kfunk edited edge metadata.

Something's fishy here. This is not in the correct scope is it? If I understand correctly, then there can only be one call to QProcess::execute(command) per run? This can't be correct as there may be multiple format lines in a format file; each of them executing a different command. Please check.

@kfunk, we should execute only one command per run - it's the original script behavior. It has simple sense - if we found some match we should apply only this command because we have "specialized" wildcards for some custom cases (which placed at the beginning of format file) and (usually) one "global" command ("*" wildcard) for all other cases.

I think it looks okay now, KDevFormatFile::apply makes it more explicit: there is a 1:1 relationship between format lines and command execution. For each format line, there's one call to QProcess::execute eventually => ok.

I'd still prefer if we could add a unit test here so we can be sure we don't break behavior now and in future.

This revision now requires changes to proceed.Oct 17 2016, 6:50 AM

I'd still prefer if we could add a unit test here so we can be sure we don't break behavior now and in future.

Ok, I will update the patch later with such tests.

antonanikin updated this revision to Diff 7487.Oct 18 2016, 6:37 AM
antonanikin edited edge metadata.
  1. Add tests
  1. Fix command execution - now we can use system shell capabilities, for example cat ${ORIGFILE} | grep ...

I'd still prefer if we could add a unit test here so we can be sure we don't break behavior now and in future.

@kfunk, I add tests but now they can be executed only on unix systems.

antonanikin updated this revision to Diff 7489.Oct 18 2016, 6:52 AM
antonanikin edited edge metadata.

Simplify code a bit

kfunk added inline comments.Oct 18 2016, 7:10 AM
util/tests/test_kdevformatsource.cpp
33

Hmm, test looks useful, but it would be more easy to understand if you'd have used data-driven tests here. In the data section, you could both specify the format file contents + expected contents for the source files.

Right know we're duplicating a lot of code in the test functions.

I'll give it a runtime ASAP, then I can give the final 'go'

util/kdevformatfile.cpp
156

#ifdef Q_OS_WIN? (Qt way)

util/tests/test_kdevformatsource.cpp
33

Typo: TestKdevForamtSource

33

I actually meant the way Qt implements data-driven testing, see here: http://doc.qt.io/qt-5/qttestlib-tutorial2-example.html

But feel free to leave it like that, I'll refactor it if noone else steps up.

fix format_sources file parsing - commands may contains : character - see updated testMatch1()

antonanikin marked 2 inline comments as done.
antonanikin added inline comments.
util/tests/test_kdevformatsource.cpp
33

Ok. I'll try to use this way in next revision.

antonanikin updated this revision to Diff 7513.Oct 18 2016, 3:49 PM

Data Driven Testing

antonanikin marked 3 inline comments as done.Oct 18 2016, 3:50 PM
antonanikin updated this revision to Diff 7528.Oct 19 2016, 3:17 AM

small codestyle fixes

This revision was automatically updated to reflect the committed changes.