sitter added a comment.
This is starting to look really good. All functions will need documenting in
the header of that file so they show up on api.kde.org, see other modules for
examples.
INLINE COMMENTS
> ECMSourceVersionControl.cmake:61
> +
> +set(_ECM_SOURCE_VERSION_CONTROL_ALREADY_WARNED_NOT_VCS FALSE)
> +set(_ECM_SOURCE_VERSION_CONTROL_ALREADY_WARNED_MISSING_GIT_EXEC FALSE)
Do we really need this? It seems to me we could just spam it for every call, in
the grand scheme of things it makes no difference but is less logic one has to
worry about when extending this module.
> ECMSourceVersionControl.cmake:75
> + # Git
> + if(NOT _ECM_SOURCE_VERSION_GIT_EXECUTABLE)
> + find_program(_ECM_SOURCE_VERSION_GIT_EXECUTABLE
I'd move the exec detection and missing reporting into its own helper which
gets called by all "public" functions. Currently the logic is duped in both
functions.
> ECMSourceVersionControl.cmake:87
> + )
> + set(ECM_SOURCE_VERSION_CONTROL_REVISION
> "${ECM_SOURCE_VERSION_CONTROL_REVISION}" PARENT_SCOPE)
> + else()
I think it's more idiomatic if you accept the variable name as a function
argument instead of hardcoding one.
For example `find_program`, but really most helpers work like that off the top
of my head.
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D24641
To: thomasfischer, sitter, kossebau
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy,
michaelh, ngraham, bruns
