[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:       Aleix Pol <aleixpol () kde ! org>
Date:       2021-04-06 11:59:40
Message-ID: CACcA1RpbtudXTnTHuafKZxYS==n5osBdPjsK1YHL3azeDgpTDg () mail ! gmail ! com
[Download RAW message or body]

On Tue, Apr 6, 2021 at 11:08 AM Harald Sitter <sitter@kde.org> wrote:

> - 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
>

I think all the issues brought up here have been addressed.

Thanks everyone!
Aleix

[Attachment #3 (text/html)]

<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Apr \
6, 2021 at 11:08 AM Harald Sitter &lt;<a \
href="mailto:sitter@kde.org">sitter@kde.org</a>&gt; wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex">- I&#39;m like a broken record at this point: some \
of the cmake files and<br> the json file lack licensing details. please run `reuse \
lint` to check<br> the list. it&#39;s incredibly close to being fully licensed<br>
- you include KDEClangFormat but don&#39;t actually seem to enable it<br>
- along a similar note: the code style is inconsistent at times, you<br>
might want to also enable the githooks when built with a new enough<br>
ECM<br>
- CheckIncludeFiles is also included but not used<br>
- you sometimes use the Qt5::Foo targets rather than the Qt::Foo<br>
targets, I believe the latter is preferred for easier porting to qt6<br>
- the library code calls qputenv and ignores its return value, might<br>
make sense to qcwarning when the env variable failed to set<br>
<br>
other than that it looks lovely 👍<br>
<br>
HS<br></blockquote><div><br></div><div style="font-size:small" \
class="gmail_default">I think all the issues brought up here have been \
addressed.</div><div style="font-size:small" class="gmail_default"><br></div><div \
style="font-size:small" class="gmail_default">Thanks everyone!</div><div \
style="font-size:small" class="gmail_default">Aleix</div></div></div>



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