D20032: Convert string formatting tests to be data driven

This revision was automatically updated to reflect the committed changes. Closed by commit R286:4d48330829d9: Convert string formatting tests to be data driven (authored by bruns). REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20032?vs=55073=55322

D20032: Convert string formatting tests to be data driven

bruns added a comment. In D20032#442635 , @apol wrote: > Whatever. > @bruns Have you considered becoming the module maintainer? :) Thats fine for me. Where do I have to deliver my soul? ;-) REPOSITORY R286 KFileMetaData BRANCH

D20032: Convert string formatting tests to be data driven

apol accepted this revision. apol added a comment. This revision is now accepted and ready to land. Whatever. @bruns Have you considered becoming the module maintainer? :) REPOSITORY R286 KFileMetaData BRANCH submit REVISION DETAIL https://phabricator.kde.org/D20032 To: bruns,

D20032: Convert string formatting tests to be data driven

bruns added a comment. Ping! REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D20032 To: bruns, #baloo, #frameworks, ngraham, astippich Cc: mgallien, apol, kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns,

D20032: Convert string formatting tests to be data driven

bruns added inline comments. INLINE COMMENTS > apol wrote in propertyinfotest.cpp:71 > Not really, but if @mgallien, who is the maintainer, is fine with it, I'm > fine too. > After all it's still better than what we used to have. @mgallien has not done any coding are reviews on kfilemetadata

D20032: Convert string formatting tests to be data driven

bruns updated this revision to Diff 55073. bruns added a comment. rebase REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20032?vs=54741=55073 BRANCH submit REVISION DETAIL https://phabricator.kde.org/D20032 AFFECTED FILES

D20032: Convert string formatting tests to be data driven

apol added a subscriber: mgallien. apol added inline comments. INLINE COMMENTS > bruns wrote in propertyinfotest.cpp:71 > Still not convinced? Not really, but if @mgallien, who is the maintainer, is fine with it, I'm fine too. After all it's still better than what we used to have. REPOSITORY

D20032: Convert string formatting tests to be data driven

bruns added a comment. @apol - do you have a proposal how to avoid the "weird struct" and still keep it readable? Preferably, no repetition of the Property, and no extra explicit constructors. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D20032 To: bruns,

D20032: Convert string formatting tests to be data driven

bruns added a comment. Ping! REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D20032 To: bruns, #baloo, #frameworks, ngraham, astippich Cc: apol, kde-frameworks-devel, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D20032: Convert string formatting tests to be data driven

bruns added inline comments. INLINE COMMENTS > bruns wrote in propertyinfotest.cpp:71 > First 2 rows expanded, just for you: > > QTest::addRow("%s", > PropertyInfo(DiscNumber).displayName().toUtf8().constData()) << > PropertyInfo(Property::DiscNumber) << true << QVariant(2018) << >

D20032: Convert string formatting tests to be data driven

apol added inline comments. INLINE COMMENTS > bruns wrote in propertyinfotest.cpp:71 > This saves adding the `QVariant(...)` around each value, and avoids the > repeated formatting of the row name/data index. The property enum is used > twice in each addRow. And instead it makes you create a

D20032: Convert string formatting tests to be data driven

bruns added inline comments. INLINE COMMENTS > apol wrote in propertyinfotest.cpp:71 > And instead it makes you create a weird local struct and loop to feed to > QTest. I don't find it very convincing. First 2 rows expanded, just for you: QTest::addRow("%s",

D20032: Convert string formatting tests to be data driven

bruns added inline comments. INLINE COMMENTS > apol wrote in propertyinfotest.cpp:71 > Is the struct really necessary? > You can be about as short by calling QTest::addRow directly for each line. This saves adding the `QVariant(...)` around each value, and avoids the repeated formatting of the

D20032: Convert string formatting tests to be data driven

apol added a comment. +1 to _data splitting, makes a lot of sense. INLINE COMMENTS > propertyinfotest.cpp:71 > > -PropertyInfo bitRate(Property::BitRate); > -QCOMPARE(bitRate.formatAsDisplayString(QVariant(128000)), > QStringLiteral("128 kbit/s")); > -

D20032: Convert string formatting tests to be data driven

bruns added a dependent revision: D20033: Default string formatting test to C locale, add localized run. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D20032 To: bruns, #baloo, #frameworks, ngraham, astippich Cc: kde-frameworks-devel, gennad, domson,

D20032: Convert string formatting tests to be data driven

bruns created this revision. bruns added reviewers: Baloo, Frameworks, ngraham, astippich. Herald added projects: Frameworks, Baloo. Herald added a subscriber: kde-frameworks-devel. bruns requested review of this revision. REVISION SUMMARY Less boilerplate code in the actual test data.