D16032: Generate all kdebugsettings .categories files automatically

[Kevin Funk]

kfunk added a comment.


  I'm torn about this review; I'm with Aleix in the sense that this adds lots and lots of extra CMake code to maintain without a /lot/ of gain.

INLINE COMMENTS

> CMakeLists.txt:185
>  # kdebugsettings file
> -install(FILES kdevelop.categories DESTINATION ${KDE_INSTALL_CONFDIR})
> +install_app_plugin_qt_logging_categories()
>  

I think it's over the top to factor that out into a reusable function if it's only going to be used once. Would revert to the previous version.

> CMakeLists.txt:14
>  
> -ecm_qt_declare_logging_category(kdevelop_SRCS
> -    HEADER debug.h
> +declare_app_qt_logging_category(kdevelop_SRCS
>      IDENTIFIER APP

Dito: I think it's over the top to factor that out into a reusable function if it's only going to be used once. Would revert to the previous version.

> KDevelopMacrosInternal.cmake:143
> +#   )
> +macro(declare_platformlib_qt_logging_category _sources)
> +    set(options )

Way too much code duplication in all those `declare_*_qt_logging_category` calls, IMO... But I realize that if you want to keep `_declare_qt_logging_category` KDevelop-agnostic (for possible future upstreaming to ECM...) there's no way around this.

If you don't want to keep `_declare_qt_logging_category` KDevelop-agnostic you could add a `TYPE` argument to it and set some defaults for the different `ARGS_*` based on `TYPE`  instead. Thinking this out further, we wouldn't even need different `declare_*_qt_logging_category`functions, but just use a `declare_qt_logging_category` function with a `TYPE` parameter directly. Just specifiy `TYPE ...` at the call-site in the individual CMakeLists.txt files...

> KDevelopMacrosInternal.cmake:143
> +#   )
> +macro(declare_platformlib_qt_logging_category _sources)
> +    set(options )

Any reason this is `macro()`? Could be a `function()`, no?

> KDevelopMacrosInternal.cmake:143
> +#   )
> +macro(declare_platformlib_qt_logging_category _sources)
> +    set(options )

Style (sth for a future commit maybe): `_sources` -> `sources` or `sources_var` (to indicate it's an out var)

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D16032

To: kossebau, #kdevelop
Cc: kfunk, apol, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20181011/a724ec7f/attachment.html>