[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-02-05 14:36:26
Message-ID: CAHAc2jccHBQaRjkxZq1DNhSPZGCaTh6FirveQ4azepUUofuuKw () mail ! gmail ! com
[Download RAW message or body]
Hi Steve,
I've reviewed what got merged from PR2 and PR3, and reworked PR4 into what
I hope is a much more manageable form. The results are in PR9:
https://github.com/ShaheedHaque/extra-cmake-modules/pull/9
As the comments say, I recommend reviewing the changes commit-by-commit,
and providing comments directly via github. When we are happy/ready, I'll
do the merge to KDE. I'll then redo the other PRs in turn, and bring them
back for review: in other words, for now, please only consider this one
pull request PR9.
On 4 February 2017 at 18:01, Shaheed Haque <srhaque@theiet.org> wrote:
> Hi Steve,
>
> Thanks for taking a look. I'll take another look at the PR4: I think I can
> split a few bits out in a more granular fashion and that should make it
> easier for you.
>
> That said, I would prefer to take the PRs in the order I posted them
> because otherwise it is hard for me to track what is and is not "done",
> especially as you sometimes end up changing what actually gets committed to
> KDE/master; that effectively creates yet another fork for me to have to
> reconcile. Ideally, I'd like us to use the PR process and let me do the
> commit to KDE when we are happy: the whole point of having all the branches
> and PRs split out the way they are is to make this tractable and to allow
> me to incorporate your comments in a systematic manner. As we deal with
> each PR, I can rebase and rewrite the subsequent PRs.
>
> I hope that sounds OK.
>
> Shaheed
>
> On 4 February 2017 at 12:09, Stephen Kelly <steveire@gmail.com> wrote:
>
>> Stephen Kelly wrote:
>>
>> > Simple, short and noiseless commits are easy to review, so please see if
>> > you can follow suit there.
>>
>> In particular, I tried to review the commit "Rule database structure
>> changes", but it seems to contain lots of unrelated things such as
>> outputting 'discarded' instead of 'suppressed', and lots more. That is all
>> noise and makes the commit hard to review.
>>
>> Please reduce that commit to the change in rules_SipTest.py plus whatever
>> minimum change is needed to the python code to support that. The rest of
>> the
>> changes can either be follow-ups or can appear before the API change
>> commit.
>>
>> Thanks,
>>
>> Steve.
>>
>
>
[Attachment #3 (text/html)]
<div dir="ltr"><div>Hi Steve,<br><br>I've reviewed what got merged from PR2 and \
PR3, and reworked PR4 into what I hope is a much more manageable form. The results \
are in PR9:<br><br><a \
href="https://github.com/ShaheedHaque/extra-cmake-modules/pull/9">https://github.com/ShaheedHaque/extra-cmake-modules/pull/9</a><br><br></div>As \
the comments say, I recommend reviewing the changes commit-by-commit, and providing \
comments directly via github. When we are happy/ready, I'll do the merge to KDE. \
I'll then redo the other PRs in turn, and bring them back for review: in other \
words, for now, please only consider this one pull request PR9.<br><br></div><div \
class="gmail_extra"><br><div class="gmail_quote">On 4 February 2017 at 18:01, Shaheed \
Haque <span dir="ltr"><<a href="mailto:srhaque@theiet.org" \
target="_blank">srhaque@theiet.org</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div dir="ltr"><div>Hi Steve,<br><br>Thanks for taking a \
look. I'll take another look at the PR4: I think I can split a few bits out in a \
more granular fashion and that should make it easier for you.<br><br></div><div>That \
said, I would prefer to take the PRs in the order I posted them because otherwise it \
is hard for me to track what is and is not "done", especially as you \
sometimes end up changing what actually gets committed to KDE/master; that \
effectively creates yet another fork for me to have to reconcile. Ideally, I'd \
like us to use the PR process and let me do the commit to KDE when we are happy: the \
whole point of having all the branches and PRs split out the way they are is to make \
this tractable and to allow me to incorporate your comments in a systematic manner. \
As we deal with each PR, I can rebase and rewrite the subsequent \
PRs.<br><br></div><div>I hope that sounds OK.<span class="HOEnZb"><font \
color="#888888"><br></font></span></div><span class="HOEnZb"><font \
color="#888888"><div><br></div><div>Shaheed<br></div></font></span></div><div \
class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div \
class="gmail_quote">On 4 February 2017 at 12:09, Stephen Kelly <span dir="ltr"><<a \
href="mailto:steveire@gmail.com" target="_blank">steveire@gmail.com</a>></span> \
wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px \
#ccc solid;padding-left:1ex"><span>Stephen Kelly wrote:<br> <br>
> Simple, short and noiseless commits are easy to review, so please see if<br>
> you can follow suit there.<br>
<br>
</span>In particular, I tried to review the commit "Rule database structure<br>
changes", but it seems to contain lots of unrelated things such as<br>
outputting 'discarded' instead of 'suppressed', and lots more. That \
is all<br> noise and makes the commit hard to review.<br>
<br>
Please reduce that commit to the change in rules_SipTest.py plus whatever<br>
minimum change is needed to the python code to support that. The rest of the<br>
changes can either be follow-ups or can appear before the API change commit.<br>
<br>
Thanks,<br>
<br>
Steve.<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic