----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128533/#review98085 -----------------------------------------------------------
Fix it, then Ship it! I always get dizzy when I see code that is meant to test something but is itself not tested :P I am fine with it either way. Some style and functional complaints though. kde-modules/KDECMakeSettings.cmake (line 133) <https://git.reviewboard.kde.org/r/128533/#comment66054> Shouldn't there be visual hint of it being found/notfound? Otherwise how would one know that one might want to install it for testing. kde-modules/KDECMakeSettings.cmake (line 135) <https://git.reviewboard.kde.org/r/128533/#comment66055> excess space before opening bracket kde-modules/KDECMakeSettings.cmake (line 137) <https://git.reviewboard.kde.org/r/128533/#comment66056> excess space before opening bracket kde-modules/appstreamtest.cmake (line 13) <https://git.reviewboard.kde.org/r/128533/#comment66057> excess space before opening bracket kde-modules/appstreamtest.cmake (line 20) <https://git.reviewboard.kde.org/r/128533/#comment66060> This will ignore the return value of the validation. Making the ctest summary always claim success, which is weird and meh. * make test won't ever output anything useful * ctest won't output anyting useful unless run with `-V` or somesuch magic If you actually pick up the result of the validation and then `message(STATUS` on ==0 or `message(FATAL_ERROR`, I think usefulness will go up 300% by actually claiming a validation fail when there was a fail :) - Harald Sitter On Aug. 3, 2016, 11:21 p.m., Aleix Pol Gonzalez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128533/ > ----------------------------------------------------------- > > (Updated Aug. 3, 2016, 11:21 p.m.) > > > Review request for Build System, Extra Cmake Modules, KDE Frameworks, > Matthias Klumpp, Scarlett Clark, and Harald Sitter. > > > Repository: extra-cmake-modules > > > Description > ------- > > At the moment, we're validating it in build.kde.org, but I feel it will be > easier for developers to test if we do so locally. > This patch does it by seeing which `*.appdata.xml` files are being installed > and validating them. This way we can keep it generic for all KDE projects. > > > Diffs > ----- > > kde-modules/KDECMakeSettings.cmake dd37e7f > kde-modules/appstreamtest.cmake PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/128533/diff/ > > > Testing > ------- > > Tested on some projects, locally. > > > Thanks, > > Aleix Pol Gonzalez > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
