Details
- Reviewers
kfunk - Group Reviewers
KDevelop - Maniphest Tasks
- T2398: Port kdev_format_sources script to something cross-platform
- Commits
- R32:9a9bbf752999: Port kdev_format_source script to C++/Qt
R33:9a9bbf752999: Port kdev_format_source script to C++/Qt
Diff Detail
- Repository
- R33 KDevPlatform
- Lint
Lint Skipped - Unit
Unit Tests Skipped
A bit of background? Why are you doing this? (I've never used this script myself ^^')
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) :)
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. |
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.
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.
- Add tests
- 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.
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()
util/tests/test_kdevformatsource.cpp | ||
---|---|---|
33 | Ok. I'll try to use this way in next revision. |