[cmake-developers] cmake -E capabilities [attempt 2]

[Stephen Kelly]

Tobias Hunger wrote:

> Did anyone find some time for a review yet?

Hi Tobias,

I had a look through this this evening. Thanks for working on this. The 
commit adding the functionality at the end looks much better after the extra 
generator refactoring. 

Here are some review notes:

* That commit has a cmDefinitions include though that should be removed.
* There are also some methods that should be const
* and a whitespace change that should be squashed into the commit that 
introduces it
* The pretty-print flag should be removed. Aside from being a boolean trap, 
it creates a interface segregation principle violation. See eg  

 https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=23f87e81

for more.  If you want to pretty print things on the command line I suggest 

 cmake -E capabilities | jq

which will give you colorized output, or 

 cmake -E capabilities | python -mjson.tool

for something that you already have installed. See 

 http://stackoverflow.com/questions/352098/how-can-i-pretty-print-json

for more.

* The CMAKE_BUILD_WITH_CMAKE macro should be in cmcmd.cxx wrapping the 
capabilities handling:

#if defined(CMAKE_BUILD_WITH_CMAKE)
    else if (args[1] == "capabilities") {
      cmake cm;
      std::cout << cm.ReportCapabilities();
      return 0;
    }
#endif

That define should not be used in ReportCapabilities.

* Splitting the 'CMake: Refactor ExtraGenerator registration' commit into 
multiple commits would make it more reviewable, and more bisectable in the 
future.

As it is, it is doing many different things, none of which are mentioned in 
the commit message, and some of which it probably shouldn't be doing. 

For example renaming GetExtraGeneratorName to 
GetExternalMakefileProjectGeneratorName is probably not needed. If you 
really want to do it, then it should be in its own commit with its own 
commit message which justifies the change. As it is, it adds noise to the 
big commit and makes it harder to review. Minimal is always better with 
commits which do refactoring like that.

A general good rule of thumb (which helps reviews, and makes things 
bisectable in the future) is to do one thing per commit.

Thanks,

Steve.