Re: Review Request 123742: knewstuff: Add verbose ecm message when ECM isn't found.

2015-05-13 Thread Jeremy Whiting
On May 13, 2015, 7:10 a.m., Aleix Pol Gonzalez wrote: CMakeLists.txt, line 8 https://git.reviewboard.kde.org/r/123742/diff/3/?file=368712#file368712line8 I insist, feature_sumary goes at the bottom. (and is already there) Is it needed to have it twice? Yes, otherwise we only get

Re: Review Request 123742: knewstuff: Add verbose ecm message when ECM isn't found.

2015-05-13 Thread Jeremy Whiting
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123742/ --- (Updated May 13, 2015, 7 a.m.) Review request for KDE Frameworks and

Re: Review Request 123742: knewstuff: Add verbose ecm message when ECM isn't found.

2015-05-13 Thread Jeremy Whiting
On May 13, 2015, 7:10 a.m., Aleix Pol Gonzalez wrote: CMakeLists.txt, line 8 https://git.reviewboard.kde.org/r/123742/diff/3/?file=368712#file368712line8 I insist, feature_sumary goes at the bottom. (and is already there) Is it needed to have it twice? Jeremy Whiting wrote:

Re: Review Request 123742: knewstuff: Add verbose ecm message when ECM isn't found.

2015-05-13 Thread Aleix Pol Gonzalez
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123742/#review80299 --- CMakeLists.txt (line 8)

Re: Review Request 123742: knewstuff: Add verbose ecm message when ECM isn't found.

2015-05-13 Thread Jeremy Whiting
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123742/ --- (Updated May 14, 2015, 12:47 a.m.) Status -- This change has been

Re: Review Request 123742: knewstuff: Add verbose ecm message when ECM isn't found.

2015-05-13 Thread Kevin Funk
On May 13, 2015, 6:45 a.m., Kevin Funk wrote: I'd reorder the code: ``` ... include(FeatureSummary) find_package(ECM ...) set_target_properties(ECM ...) feature_summary(...) ... ``` I know that it is a bit harder to script this way , but helps code

Re: Review Request 123742: knewstuff: Add verbose ecm message when ECM isn't found.

2015-05-13 Thread Kevin Funk
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123742/#review80275 --- I'd reorder the code: ``` ... include(FeatureSummary)

Review Request 123742: knewstuff: Add verbose ecm message when ECM isn't found.

2015-05-12 Thread Jeremy Whiting
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123742/ --- Review request for KDE Frameworks and Jeremy Whiting. Repository:

Re: Review Request 123742: knewstuff: Add verbose ecm message when ECM isn't found.

2015-05-12 Thread Jeremy Whiting
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123742/ --- (Updated May 12, 2015, 6:28 p.m.) Review request for KDE Frameworks and

Re: Review Request 123742: knewstuff: Add verbose ecm message when ECM isn't found.

2015-05-12 Thread Aleix Pol Gonzalez
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123742/#review80270 --- +1 I'd like to know other people's opinion on this, as this

Re: Review Request 123742: knewstuff: Add verbose ecm message when ECM isn't found.

2015-05-12 Thread Jeremy Whiting
On May 12, 2015, 7:41 p.m., Aleix Pol Gonzalez wrote: +1 I'd like to know other people's opinion on this, as this should end up on most frameworks. Personally I'm unsure that showing the url will enlighten people, but it's better that than the terribly verbose and useless cmake

Re: Review Request 123742: knewstuff: Add verbose ecm message when ECM isn't found.

2015-05-12 Thread Aleix Pol Gonzalez
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123742/#review80264 --- CMakeLists.txt (line 32)

Re: Review Request 123742: knewstuff: Add verbose ecm message when ECM isn't found.

2015-05-12 Thread Jeremy Whiting
On May 12, 2015, 4:49 p.m., Aleix Pol Gonzalez wrote: CMakeLists.txt, line 32 https://git.reviewboard.kde.org/r/123742/diff/1/?file=368438#file368438line32 Do you think this is needed? It's REQUIRED anyway. Oops, I did this wrong, I'll update the patch in a bit at home. the point