On 01/22/2015 11:50 AM, Raffi Enficiaud wrote: > I can also do a pull request if you prefer,
As described in CONTRIBUTING.rst a patch here is preferred. I've fetched your branch from https://github.com/raffienficiaud/CMake Here are some comments. Please wrap text in the documentation blocks of the FindMatlab module to fit in 79 columns. > # find_package(Matlab > # [VERSION] > # [REQUIRED] > # [COMPONENTS component1 [component2]]) Individual find modules don't need to summarize the find_package signature. Documentation of components and versions can just refer to the :command:`find_package` command and name the options. > # .. note: > # > # The version above is the Matlab version... The note text needs to be indented to be part of the note. The same goes for all the variable and command documentation inside explicit markup directives like ".. variable::" and ".. command::". > # User defined variables > # ---------------------- This section should be called something like "Module Input Variables" and have intro wording like "Users or projects may set the following variables to configure this module behavior.". > # Variables defined by the module > # ------------------------------- This section should distinguish between cache entries and output variables. See FindJsonCpp for an example. > # Provided macros > # --------------- Generally we try to use functions with "set(... PARENT_SCOPE)". Also on the endmacro() and endfunction() calls please drop the command name. > # WARNING: this thing pollutes the CMAKE_FIND_LIBRARY_PREFIXES global > variable. > # Should it be restored afterwards? Is there a more appropriate way to do > that? > set(CMAKE_FIND_LIBRARY_PREFIXES ${CMAKE_FIND_LIBRARY_PREFIXES} > ${_matlab_lib_prefix_for_search}) This should be handled with a save/restore. > # The function arguments are: Please use definition lists instead of bullet lists for function argument documentation. The copyright year should be 2014-2015 since we've spilled into that range. There is no need for copyright notices in the Tests/RunCMake test cmake code because there no creative input in that boilerplate. Please remove all trailing whitespace and make sure the files are committed with LF newlines and not CRLF newlines (including tests). Also make sure all files end in a newline (but not trailing blank lines). Thanks, -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
