[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-release-team
Subject:    Re: Including LayerShellQt in Plasma in time for 5.22
From:       Harald Sitter <sitter () kde ! org>
Date:       2021-04-06 9:07:54
Message-ID: CAEc+18HmCS6DPCdEbD7i7JCn-7JhnFoUbKg3Hr-pVW=JeKp0ow () mail ! gmail ! com
[Download RAW message or body]

- I'm like a broken record at this point: some of the cmake files and
the json file lack licensing details. please run `reuse lint` to check
the list. it's incredibly close to being fully licensed
- you include KDEClangFormat but don't actually seem to enable it
- along a similar note: the code style is inconsistent at times, you
might want to also enable the githooks when built with a new enough
ECM
- CheckIncludeFiles is also included but not used
- you sometimes use the Qt5::Foo targets rather than the Qt::Foo
targets, I believe the latter is preferred for easier porting to qt6
- the library code calls qputenv and ignores its return value, might
make sense to qcwarning when the env variable failed to set

other than that it looks lovely =F0=9F=91=8D

HS
[prev in list] [next in list] [prev in thread] [next in thread]