[prev in list] [next in list] [prev in thread] [next in thread]
List: kwrite-devel
Subject: Re: Qt Coding Style + clang-format + kate.git
From: Dominik Haumann <dhaumann () kde ! org>
Date: 2019-08-24 19:02:25
Message-ID: CALi_srBbiXXwRMZt0z8LXp5EXg-18q0eN1n_Vcm8jzqFrcRcRg () mail ! gmail ! com
[Download RAW message or body]
Btw, we can discuss at Akademy again...
On Sat, Aug 24, 2019 at 8:53 PM Dominik Haumann <dhaumann@kde.org> wrote:
> Hi,
>
> I just applied clang-format to Kate's source code. And already looking at
> kateapp.cpp we find:
>
> KateApp::KateApp(const QCommandLineParser &args)
> - : m_args(args)
> - , m_wrapper(appSelf = this)
> - , m_docManager(this)
> - , m_adaptor(this)
> - , m_pluginManager(this)
> - , m_sessionManager(this)
> + : m_args(args),
> + m_wrapper(appSelf = this),
> + m_docManager(this),
> + m_adaptor(this),
> + m_pluginManager(this),
> + m_sessionManager(this)
>
> Is there an option for clang-format to prefer the old behavior? The
> leading comma allows us to have much cleaner diffs with better history.
>
> Let's continue:
>
> - qputenv("KATE_PID",
> QStringLiteral("%1").arg(QCoreApplication::applicationPid()).toLatin1().constData());
> + qputenv("KATE_PID",
> +
> QStringLiteral("%1").arg(QCoreApplication::applicationPid()).toLatin1().constData());
>
> Ok, although given wide-screens I think the old variant is more readable.
> It gets more apparent here:
>
> - } else if (!m_args.isSet(QStringLiteral("stdin")) &&
> (m_args.positionalArguments().count() == 0)) { // only start session if no
> files specified
> + } else if (!m_args.isSet(QStringLiteral("stdin"))
> + && (m_args.positionalArguments().count()
> + == 0)) { // only start session if no files specified
>
> I really cannot say that the new version looks better to me.
>
> Continuing:
>
> KMessageBox::sorry(activeKateMainWindow(),
> - i18n("The file '%1' could not be opened:
> it is not a normal file, it is a folder.", info.url.toString()));
> + i18n("The file '%1' could not be opened:
> it is not a normal file, "
> + "it is a folder.",
> + info.url.toString()));
>
> Not convinced.
>
> - for (const auto& window : m_mainWindows) {
> + for (const auto &window : m_mainWindows) {
>
> Now, do we want the & left or right-aligned?
>
> Let's have a look into the header file:
>
> /**
> - * Close the given \p document. If the document is modified, user
> will be asked if he wants that.
> - * \param document the document to be closed
> - * \return \e true on success, otherwise \e false
> + * Close the given \p document. If the document is modified, user
> will be asked if he wants
> + * that. \param document the document to be closed \return \e true on
> success, otherwise \e
> + * false
> */
>
> The reformatted version breaks the doxygen comment. Ok, maybe doxygen
> still gets it right, but the \return in the same line is imho totally wrong.
>
> Unfortunately with current config: -2 (rejected) from my side. Maybe there
> is a better config, but breaking the comments this way is not worth it.
>
> Best regards
> Dominik
>
> On Sat, Aug 24, 2019 at 4:31 PM Christoph Cullmann <christoph@cullmann.io>
> wrote:
>
>> On 2019-08-24 07:48, Dominik Haumann wrote:
>> > No, will do later today.
>>
>> Thanks!
>>
>> I did inspect my local result with the config again.
>>
>> The only thing I might want to change is the
>>
>> # Column width is limited to 100 in accordance with Qt Coding Style.
>> # https://wiki.qt.io/Qt_Coding_Style
>> # Note that this may be changed at some point in the future.
>> ColumnLimit: 100
>>
>> part.
>>
>> For stuff with long variable + function names, you get ugly wraps.
>>
>> In company we settled to some 240 limit.
>> No limit doesn't work well either, as otherwise a lot of stuff will
>> just be collapsed to "LONG" lines.
>>
>> Greetings
>> Christoph
>>
>> --
>> Ignorance is bliss...
>> https://cullmann.io | https://kate-editor.org
>>
>
[Attachment #3 (text/html)]
<div dir="ltr">Btw, we can discuss at Akademy again...<br></div><br><div \
class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Aug 24, 2019 at 8:53 PM \
Dominik Haumann <<a href="mailto:dhaumann@kde.org">dhaumann@kde.org</a>> \
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"><div \
dir="ltr">Hi,<br><br>I just applied clang-format to Kate's source code. And \
already looking at kateapp.cpp we find:<br><br> KateApp::KateApp(const \
QCommandLineParser &args)<br>- : m_args(args)<br>- , m_wrapper(appSelf \
= this)<br>- , m_docManager(this)<br>- , m_adaptor(this)<br>- , \
m_pluginManager(this)<br>- , m_sessionManager(this)<br>+ : \
m_args(args),<br>+ m_wrapper(appSelf = this),<br>+ \
m_docManager(this),<br>+ m_adaptor(this),<br>+ \
m_pluginManager(this),<br>+ m_sessionManager(this)<br><br>Is there an option \
for clang-format to prefer the old behavior? The leading comma allows us to have much \
cleaner diffs with better history.<br><br>Let's continue:<br><br>- \
qputenv("KATE_PID", \
QStringLiteral("%1").arg(QCoreApplication::applicationPid()).toLatin1().constData());<br>+ \
qputenv("KATE_PID",<br>+ \
QStringLiteral("%1").arg(QCoreApplication::applicationPid()).toLatin1().constData());<br><br>Ok, \
although given wide-screens I think the old variant is more readable. It gets more \
apparent here:<br><br>- } else if \
(!m_args.isSet(QStringLiteral("stdin")) && \
(m_args.positionalArguments().count() == 0)) { // only start session if no files \
specified<br>+ } else if (!m_args.isSet(QStringLiteral("stdin"))<br>+ \
&& (m_args.positionalArguments().count()<br>+ == \
0)) { // only start session if no files specified<br><br>I really cannot say that the \
new version looks better to me.<br><br>Continuing:<br><br> \
KMessageBox::sorry(activeKateMainWindow(),<br>- \
i18n("The file '%1' could not be opened: it is not a normal file, it is \
a folder.", info.url.toString()));<br>+ \
i18n("The file '%1' could not be opened: it is not a normal file, \
"<br>+ "it is a \
folder.",<br>+ \
info.url.toString()));<br><br>Not convinced.<br><br>- for (const auto& \
window : m_mainWindows) {<br>+ for (const auto &window : m_mainWindows) \
{<br><br>Now, do we want the & left or right-aligned?<br><br>Let's have a \
look into the header file:<br><br> /**<br>- * Close the given \p \
document. If the document is modified, user will be asked if he wants that.<br>- \
* \param document the document to be closed<br>- * \return \e true on success, \
otherwise \e false<br>+ * Close the given \p document. If the document is \
modified, user will be asked if he wants<br>+ * that. \param document the \
document to be closed \return \e true on success, otherwise \e<br>+ * false<br> \
*/<br><br>The reformatted version breaks the doxygen comment. Ok, maybe doxygen still \
gets it right, but the \return in the same line is imho totally \
wrong.<br><br>Unfortunately with current config: -2 (rejected) from my side. Maybe \
there is a better config, but breaking the comments this way is not worth \
it.<br><br>Best regards<br>Dominik</div><br><div class="gmail_quote"><div dir="ltr" \
class="gmail_attr">On Sat, Aug 24, 2019 at 4:31 PM Christoph Cullmann <<a \
href="mailto:christoph@cullmann.io" target="_blank">christoph@cullmann.io</a>> \
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">On 2019-08-24 07:48, \
Dominik Haumann wrote:<br> > No, will do later today.<br>
<br>
Thanks!<br>
<br>
I did inspect my local result with the config again.<br>
<br>
The only thing I might want to change is the<br>
<br>
# Column width is limited to 100 in accordance with Qt Coding Style.<br>
# <a href="https://wiki.qt.io/Qt_Coding_Style" rel="noreferrer" \
target="_blank">https://wiki.qt.io/Qt_Coding_Style</a><br> # Note that this may be \
changed at some point in the future.<br>
ColumnLimit: 100<br>
<br>
part.<br>
<br>
For stuff with long variable + function names, you get ugly wraps.<br>
<br>
In company we settled to some 240 limit.<br>
No limit doesn't work well either, as otherwise a lot of stuff will<br>
just be collapsed to "LONG" lines.<br>
<br>
Greetings<br>
Christoph<br>
<br>
-- <br>
Ignorance is bliss...<br>
<a href="https://cullmann.io" rel="noreferrer" \
target="_blank">https://cullmann.io</a> | <a href="https://kate-editor.org" \
rel="noreferrer" target="_blank">https://kate-editor.org</a><br> </blockquote></div>
</blockquote></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic