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

List:       kde-bindings
Subject:    Re: PyKF5 bindings generator improvements out for review.
From:       Shaheed Haque <srhaque () theiet ! org>
Date:       2017-01-29 16:49:50
Message-ID: CAHAc2jcQ8QVk_LvjMX+gkJ78PQu8Lv=TQdfMPN_r_ujJYdGv+Q () mail ! gmail ! com
[Download RAW message or body]

Rewrite? It think its more a case of forking making things diverge (if you
look, a lot of the changes are formatting, name changes and comments,
though I accept that does make for a lot of noise). I have tried VERY hard
to keep the good changes form both sides...and yes, there ARE a reasonable
number of actual changes too.

Anyway, I was under the impression a squashed single commit was preferred,
but I can look to pull things out again. I would prefer to work the reviews
via github's Pull Requests, is that still OK?


On 29 January 2017 at 16:24, Stephen Kelly <steveire@gmail.com> wrote:

> Shaheed Haque wrote:
>
> > All the code is squashed into the single commit. Feedback welcome here or
> > via reviewboard or via github.
>
> Hi Shaheed,
>
> Having so many changes in a single commit is not really reviewable now or
> in
> the future. Commits should be minimal so that people in the future who
> examine the history can make sense of it. The commit message for that
> commit
> says
>
>  Re-introduce the missing databases.
>
> but in fact it appears to be a complete re-write in a single commit.
>
> Please extract meaningful changes into separate commits. For example, the
> change to
>
>  find-modules/Qt5Ruleset.py
>
> must correspond to only a part of the rest of the commit. Please extract
> that into a commit. Then consider any other changes in the big commit and
> extract small meaningful commits and repeat.
>
> When reviewing commits the reader should have only one context in mind. For
> example "ok, this commit is changing the API of rulesets" or "ok, this
> commit is introducing a noop so enable silencing warnings".
>
>  Usually that context for the commit should be in the commit message!
>
> Thanks,
>
> Steve.
>

[Attachment #3 (text/html)]

<div dir="ltr">Rewrite? It think its more a case of forking making things diverge (if \
you look, a lot of the changes are formatting, name changes and comments, though I \
accept that does make for a lot of noise). I have tried VERY hard to keep the good \
changes form both sides...and yes, there ARE a reasonable number of actual changes \
too.<br><br>Anyway, I was under the impression a squashed single commit was \
preferred, but I can look to pull things out again. I would prefer to work the \
reviews via github&#39;s Pull Requests, is that still OK?<br><div \
class="gmail_extra"><br><br><div class="gmail_quote">On 29 January 2017 at 16:24, \
Stephen Kelly <span dir="ltr">&lt;<a href="mailto:steveire@gmail.com" \
target="_blank">steveire@gmail.com</a>&gt;</span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><span class="gmail-">Shaheed Haque wrote:<br> <br>
&gt; All the code is squashed into the single commit. Feedback welcome here or<br>
&gt; via reviewboard or via github.<br>
<br>
</span>Hi Shaheed,<br>
<br>
Having so many changes in a single commit is not really reviewable now or in<br>
the future. Commits should be minimal so that people in the future who<br>
examine the history can make sense of it. The commit message for that commit<br>
says<br>
<br>
  Re-introduce the missing databases.<br>
<br>
but in fact it appears to be a complete re-write in a single commit.<br>
<br>
Please extract meaningful changes into separate commits. For example, the<br>
change to<br>
<br>
  find-modules/Qt5Ruleset.py<br>
<br>
must correspond to only a part of the rest of the commit. Please extract<br>
that into a commit. Then consider any other changes in the big commit and<br>
extract small meaningful commits and repeat.<br>
<br>
When reviewing commits the reader should have only one context in mind. For<br>
example &quot;ok, this commit is changing the API of rulesets&quot; or &quot;ok, \
this<br> commit is introducing a noop so enable silencing warnings&quot;.<br>
<br>
  Usually that context for the commit should be in the commit message!<br>
<br>
Thanks,<br>
<br>
Steve.<br>
</blockquote></div><br></div></div>



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