Benjamin Reed
2011-10-31, 21:06
Ted Dunning
2011-10-31, 21:10
Benjamin Reed
2011-10-31, 21:15
Ted Dunning
2011-10-31, 21:18
Thomas Koch
2011-10-31, 21:35
Ted Dunning
2011-10-31, 21:39
Thomas Koch
2011-10-31, 22:11
Ted Dunning
2011-10-31, 22:26
Patrick Hunt
2011-10-31, 22:51
Ted Dunning
2011-10-31, 23:08
Benjamin Reed
2011-10-31, 23:28
Ted Dunning
2011-10-31, 23:30
Patrick Hunt
2011-10-31, 23:31
Ted Dunning
2011-10-31, 23:43
Mahadev Konar
2011-11-01, 01:47
Benjamin Reed
2011-11-01, 04:39
Fournier, Camille F.
2011-11-01, 14:21
Patrick Hunt
2011-11-01, 15:40
Thomas Koch
2011-11-01, 16:11
Fournier, Camille F.
2011-11-01, 16:51
Patrick Hunt
2011-11-01, 17:15
Patrick Hunt
2011-11-01, 17:32
Fournier, Camille F.
2011-11-01, 18:31
Fournier, Camille F.
2011-11-01, 18:39
Patrick Hunt
2011-11-01, 18:43
-
cleanup and subjective patchesBenjamin Reed 2011-10-31, 21:06
i know we have discussed this in the past, but we never really came to
a consensus or policy, so i'd like to reopen the discussion on cleanup
and subjective patches.
currently there are 7 jiras for cleanup issues, 6 of them marked as
major. the problem with cleanup or subjective patches is that they
take committer time, they can collide with other patches (or patches
in progress), and they can introduce new bugs. i know we have
discussed this in the past, but i think it would be good to establish
a policy of rejecting such patches. this isn't to say that the code
looks nice or is clean as it is, it's just that we want to avoid
disrupting a code base that people count on.
that is my perspective. it would be good to come to an agreement on a
policy so that we can deal with such patches quickly rather than drag
things out for both the contributor and the committer. right now, we
have a patch that fixes a memory leak and a patch that switches
integer constants to enum and they are both marked as major. another
minor patch fixes how the log cleanups are done. i think we should be
focusing on the leaks and logs. perhaps in the future when we don't
have such a backlog of changes, we can reevaluate the policy, but
right now i would like to make "code cleanup not a bug fix or
enhancement" a reason for patch rejection.
other thoughts?
thanx
ben
a consensus or policy, so i'd like to reopen the discussion on cleanup
and subjective patches.
currently there are 7 jiras for cleanup issues, 6 of them marked as
major. the problem with cleanup or subjective patches is that they
take committer time, they can collide with other patches (or patches
in progress), and they can introduce new bugs. i know we have
discussed this in the past, but i think it would be good to establish
a policy of rejecting such patches. this isn't to say that the code
looks nice or is clean as it is, it's just that we want to avoid
disrupting a code base that people count on.
that is my perspective. it would be good to come to an agreement on a
policy so that we can deal with such patches quickly rather than drag
things out for both the contributor and the committer. right now, we
have a patch that fixes a memory leak and a patch that switches
integer constants to enum and they are both marked as major. another
minor patch fixes how the log cleanups are done. i think we should be
focusing on the leaks and logs. perhaps in the future when we don't
have such a backlog of changes, we can reevaluate the policy, but
right now i would like to make "code cleanup not a bug fix or
enhancement" a reason for patch rejection.
other thoughts?
thanx
ben
-
Re: cleanup and subjective patchesTed Dunning 2011-10-31, 21:10
This is a bold position.
I can't disagree with the prioritization. And I can't disagree with the
risk assessment (see
https://issues.apache.org/jira/browse/ZOOKEEPER-1271for an example
where my fixing one bug caused another).
But summarily rejecting all such changes seems a bit harsh. I think it is
fine to downgrade the severity on all cleanup JIRA's. Rejecting them out
of hand is a good way to guarantee (in my opinion) that ZK eventually dies
of bit-rot.
On Mon, Oct 31, 2011 at 2:06 PM, Benjamin Reed <[EMAIL PROTECTED]> wrote:
> i know we have discussed this in the past, but we never really came to
> a consensus or policy, so i'd like to reopen the discussion on cleanup
> and subjective patches.
>
> currently there are 7 jiras for cleanup issues, 6 of them marked as
> major. the problem with cleanup or subjective patches is that they
> take committer time, they can collide with other patches (or patches
> in progress), and they can introduce new bugs. i know we have
> discussed this in the past, but i think it would be good to establish
> a policy of rejecting such patches. this isn't to say that the code
> looks nice or is clean as it is, it's just that we want to avoid
> disrupting a code base that people count on.
>
> that is my perspective. it would be good to come to an agreement on a
> policy so that we can deal with such patches quickly rather than drag
> things out for both the contributor and the committer. right now, we
> have a patch that fixes a memory leak and a patch that switches
> integer constants to enum and they are both marked as major. another
> minor patch fixes how the log cleanups are done. i think we should be
> focusing on the leaks and logs. perhaps in the future when we don't
> have such a backlog of changes, we can reevaluate the policy, but
> right now i would like to make "code cleanup not a bug fix or
> enhancement" a reason for patch rejection.
>
> other thoughts?
>
> thanx
> ben
>
I can't disagree with the prioritization. And I can't disagree with the
risk assessment (see
https://issues.apache.org/jira/browse/ZOOKEEPER-1271for an example
where my fixing one bug caused another).
But summarily rejecting all such changes seems a bit harsh. I think it is
fine to downgrade the severity on all cleanup JIRA's. Rejecting them out
of hand is a good way to guarantee (in my opinion) that ZK eventually dies
of bit-rot.
On Mon, Oct 31, 2011 at 2:06 PM, Benjamin Reed <[EMAIL PROTECTED]> wrote:
> i know we have discussed this in the past, but we never really came to
> a consensus or policy, so i'd like to reopen the discussion on cleanup
> and subjective patches.
>
> currently there are 7 jiras for cleanup issues, 6 of them marked as
> major. the problem with cleanup or subjective patches is that they
> take committer time, they can collide with other patches (or patches
> in progress), and they can introduce new bugs. i know we have
> discussed this in the past, but i think it would be good to establish
> a policy of rejecting such patches. this isn't to say that the code
> looks nice or is clean as it is, it's just that we want to avoid
> disrupting a code base that people count on.
>
> that is my perspective. it would be good to come to an agreement on a
> policy so that we can deal with such patches quickly rather than drag
> things out for both the contributor and the committer. right now, we
> have a patch that fixes a memory leak and a patch that switches
> integer constants to enum and they are both marked as major. another
> minor patch fixes how the log cleanups are done. i think we should be
> focusing on the leaks and logs. perhaps in the future when we don't
> have such a backlog of changes, we can reevaluate the policy, but
> right now i would like to make "code cleanup not a bug fix or
> enhancement" a reason for patch rejection.
>
> other thoughts?
>
> thanx
> ben
>
-
Re: cleanup and subjective patchesBenjamin Reed 2011-10-31, 21:15
deprioritizing them doesn't help because the patches themselves bit
rot. shortly they will not apply and then they will be worthless. the
poor contributor would then be left with the task of maintaining a
patch that would never commit.
note also that i'm not saying that the code should not be cleaned.
it's just that it should be done in conjunction with an enhancement or
bug fix.
ben
On Mon, Oct 31, 2011 at 2:10 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
> This is a bold position.
>
> I can't disagree with the prioritization. And I can't disagree with the
> risk assessment (see
> https://issues.apache.org/jira/browse/ZOOKEEPER-1271for an example
> where my fixing one bug caused another).
>
> But summarily rejecting all such changes seems a bit harsh. I think it is
> fine to downgrade the severity on all cleanup JIRA's. Rejecting them out
> of hand is a good way to guarantee (in my opinion) that ZK eventually dies
> of bit-rot.
>
> On Mon, Oct 31, 2011 at 2:06 PM, Benjamin Reed <[EMAIL PROTECTED]> wrote:
>
>> i know we have discussed this in the past, but we never really came to
>> a consensus or policy, so i'd like to reopen the discussion on cleanup
>> and subjective patches.
>>
>> currently there are 7 jiras for cleanup issues, 6 of them marked as
>> major. the problem with cleanup or subjective patches is that they
>> take committer time, they can collide with other patches (or patches
>> in progress), and they can introduce new bugs. i know we have
>> discussed this in the past, but i think it would be good to establish
>> a policy of rejecting such patches. this isn't to say that the code
>> looks nice or is clean as it is, it's just that we want to avoid
>> disrupting a code base that people count on.
>>
>> that is my perspective. it would be good to come to an agreement on a
>> policy so that we can deal with such patches quickly rather than drag
>> things out for both the contributor and the committer. right now, we
>> have a patch that fixes a memory leak and a patch that switches
>> integer constants to enum and they are both marked as major. another
>> minor patch fixes how the log cleanups are done. i think we should be
>> focusing on the leaks and logs. perhaps in the future when we don't
>> have such a backlog of changes, we can reevaluate the policy, but
>> right now i would like to make "code cleanup not a bug fix or
>> enhancement" a reason for patch rejection.
>>
>> other thoughts?
>>
>> thanx
>> ben
>>
>
rot. shortly they will not apply and then they will be worthless. the
poor contributor would then be left with the task of maintaining a
patch that would never commit.
note also that i'm not saying that the code should not be cleaned.
it's just that it should be done in conjunction with an enhancement or
bug fix.
ben
On Mon, Oct 31, 2011 at 2:10 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
> This is a bold position.
>
> I can't disagree with the prioritization. And I can't disagree with the
> risk assessment (see
> https://issues.apache.org/jira/browse/ZOOKEEPER-1271for an example
> where my fixing one bug caused another).
>
> But summarily rejecting all such changes seems a bit harsh. I think it is
> fine to downgrade the severity on all cleanup JIRA's. Rejecting them out
> of hand is a good way to guarantee (in my opinion) that ZK eventually dies
> of bit-rot.
>
> On Mon, Oct 31, 2011 at 2:06 PM, Benjamin Reed <[EMAIL PROTECTED]> wrote:
>
>> i know we have discussed this in the past, but we never really came to
>> a consensus or policy, so i'd like to reopen the discussion on cleanup
>> and subjective patches.
>>
>> currently there are 7 jiras for cleanup issues, 6 of them marked as
>> major. the problem with cleanup or subjective patches is that they
>> take committer time, they can collide with other patches (or patches
>> in progress), and they can introduce new bugs. i know we have
>> discussed this in the past, but i think it would be good to establish
>> a policy of rejecting such patches. this isn't to say that the code
>> looks nice or is clean as it is, it's just that we want to avoid
>> disrupting a code base that people count on.
>>
>> that is my perspective. it would be good to come to an agreement on a
>> policy so that we can deal with such patches quickly rather than drag
>> things out for both the contributor and the committer. right now, we
>> have a patch that fixes a memory leak and a patch that switches
>> integer constants to enum and they are both marked as major. another
>> minor patch fixes how the log cleanups are done. i think we should be
>> focusing on the leaks and logs. perhaps in the future when we don't
>> have such a backlog of changes, we can reevaluate the policy, but
>> right now i would like to make "code cleanup not a bug fix or
>> enhancement" a reason for patch rejection.
>>
>> other thoughts?
>>
>> thanx
>> ben
>>
>
-
Re: cleanup and subjective patchesTed Dunning 2011-10-31, 21:18
Is that any better?
In some sense, it can be said to be worse because the enhancement or bug
fix would be the focus of review. That could make it less likely that the
bugs caused by the aesthetic changes are overlooked.
On Mon, Oct 31, 2011 at 2:15 PM, Benjamin Reed <[EMAIL PROTECTED]> wrote:
> note also that i'm not saying that the code should not be cleaned.
> it's just that it should be done in conjunction with an enhancement or
> bug fix.
>
In some sense, it can be said to be worse because the enhancement or bug
fix would be the focus of review. That could make it less likely that the
bugs caused by the aesthetic changes are overlooked.
On Mon, Oct 31, 2011 at 2:15 PM, Benjamin Reed <[EMAIL PROTECTED]> wrote:
> note also that i'm not saying that the code should not be cleaned.
> it's just that it should be done in conjunction with an enhancement or
> bug fix.
>
-
Re: cleanup and subjective patchesThomas Koch 2011-10-31, 21:35
Thanks to Ted for replying. I will save the mail I started in the drafts
folder until I've calmed down again.
Benjamin Reed:
> deprioritizing them doesn't help because the patches themselves bit
> rot. shortly they will not apply and then they will be worthless. the
> poor contributor would then be left with the task of maintaining a
> patch that would never commit.
You should really give GIT a try. I've kept a pipeline of half a douzend
patches filled and current over the last two months while drinking my morning
coffee.
Likewise have I updated my development branch of over a douzend commits every
morning against the new ZK trunk.
Regards,
Thomas Koch, http://www.koch.ro
folder until I've calmed down again.
Benjamin Reed:
> deprioritizing them doesn't help because the patches themselves bit
> rot. shortly they will not apply and then they will be worthless. the
> poor contributor would then be left with the task of maintaining a
> patch that would never commit.
You should really give GIT a try. I've kept a pipeline of half a douzend
patches filled and current over the last two months while drinking my morning
coffee.
Likewise have I updated my development branch of over a douzend commits every
morning against the new ZK trunk.
Regards,
Thomas Koch, http://www.koch.ro
-
Re: cleanup and subjective patchesTed Dunning 2011-10-31, 21:39
Jumping over to Ben's side of the discussion,
Git helps with this, but does not eliminate the problem. At some point the
changes become difficult to understand relative to the new code and may
even collide in ways that are difficult to merge.
It is true, however, that patches can be kept live using tools like git.
That is how I kept the multi stuff alive, but there was at least one
tricky merge to be done.
On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
> Thanks to Ted for replying. I will save the mail I started in the drafts
> folder until I've calmed down again.
>
> Benjamin Reed:
> > deprioritizing them doesn't help because the patches themselves bit
> > rot. shortly they will not apply and then they will be worthless. the
> > poor contributor would then be left with the task of maintaining a
> > patch that would never commit.
> You should really give GIT a try. I've kept a pipeline of half a douzend
> patches filled and current over the last two months while drinking my
> morning
> coffee.
> Likewise have I updated my development branch of over a douzend commits
> every
> morning against the new ZK trunk.
>
> Regards,
>
> Thomas Koch, http://www.koch.ro
>
Git helps with this, but does not eliminate the problem. At some point the
changes become difficult to understand relative to the new code and may
even collide in ways that are difficult to merge.
It is true, however, that patches can be kept live using tools like git.
That is how I kept the multi stuff alive, but there was at least one
tricky merge to be done.
On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
> Thanks to Ted for replying. I will save the mail I started in the drafts
> folder until I've calmed down again.
>
> Benjamin Reed:
> > deprioritizing them doesn't help because the patches themselves bit
> > rot. shortly they will not apply and then they will be worthless. the
> > poor contributor would then be left with the task of maintaining a
> > patch that would never commit.
> You should really give GIT a try. I've kept a pipeline of half a douzend
> patches filled and current over the last two months while drinking my
> morning
> coffee.
> Likewise have I updated my development branch of over a douzend commits
> every
> morning against the new ZK trunk.
>
> Regards,
>
> Thomas Koch, http://www.koch.ro
>
-
Re: cleanup and subjective patchesThomas Koch 2011-10-31, 22:11
Benjamin Reed:
> i know we have discussed this in the past, but we never really came to
> a consensus or policy, so i'd like to reopen the discussion on cleanup
> and subjective patches.
Just one question: Could you please try to elaborate on how a non subjective
distinction could be made between "subjective" and "non-subjective" patches?
Regards,
Thomas Koch, http://www.koch.ro
> i know we have discussed this in the past, but we never really came to
> a consensus or policy, so i'd like to reopen the discussion on cleanup
> and subjective patches.
Just one question: Could you please try to elaborate on how a non subjective
distinction could be made between "subjective" and "non-subjective" patches?
Regards,
Thomas Koch, http://www.koch.ro
-
Re: cleanup and subjective patchesTed Dunning 2011-10-31, 22:26
One test for this is to note when reasonable experts disagree on the
correct action on a non-functional basis.
A key example is indentation. Nobody worth listening to makes an argument
that bad indentation or the use of tabs directly causes a bug.
A counter example is a behavior that is demonstrated by a unit test that is
generally agreed to be incorrect.
This process is obviously grounded in subjective meta-decisions, but the
Zookeeper is probably not the right place to go too deeply into
epistemology and the symbol grounding problem.
Another test is to ask the community. If the community has consensus, then
that is likely the right now. If the community does not have substantial
preponderance of one opinion over another, then ask Ben. If you still
disagree with the answer, you should probably think deeply about
epistemology and speaker/listener symmetry, but you should do your thinking
off-list.
On Mon, Oct 31, 2011 at 3:11 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
> Benjamin Reed:
> > i know we have discussed this in the past, but we never really came to
> > a consensus or policy, so i'd like to reopen the discussion on cleanup
> > and subjective patches.
> Just one question: Could you please try to elaborate on how a non
> subjective
> distinction could be made between "subjective" and "non-subjective"
> patches?
>
> Regards,
>
> Thomas Koch, http://www.koch.ro
>
correct action on a non-functional basis.
A key example is indentation. Nobody worth listening to makes an argument
that bad indentation or the use of tabs directly causes a bug.
A counter example is a behavior that is demonstrated by a unit test that is
generally agreed to be incorrect.
This process is obviously grounded in subjective meta-decisions, but the
Zookeeper is probably not the right place to go too deeply into
epistemology and the symbol grounding problem.
Another test is to ask the community. If the community has consensus, then
that is likely the right now. If the community does not have substantial
preponderance of one opinion over another, then ask Ben. If you still
disagree with the answer, you should probably think deeply about
epistemology and speaker/listener symmetry, but you should do your thinking
off-list.
On Mon, Oct 31, 2011 at 3:11 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
> Benjamin Reed:
> > i know we have discussed this in the past, but we never really came to
> > a consensus or policy, so i'd like to reopen the discussion on cleanup
> > and subjective patches.
> Just one question: Could you please try to elaborate on how a non
> subjective
> distinction could be made between "subjective" and "non-subjective"
> patches?
>
> Regards,
>
> Thomas Koch, http://www.koch.ro
>
-
Re: cleanup and subjective patchesPatrick Hunt 2011-10-31, 22:51
EOD this is not a tool problem, it's a resource issue. We have limited
committer time available for reviews. IMO Thomas's changes have been
pretty mechanical so far, they are easy to review/commit. They do
result in some patch churn but I've been surprised it's been so little
so far and the code is improving as a result. However we've just
scratched the surface (the easy stuff), I do have some concerns as we
make more significant structural changes. Personally I think we need
to focus on beefing up testing prior to making more significant
changes.
Ben - We won't get new committers if we limit contributions. We'll
just dig a deeper hole for ourselves. Granted if we only review/accept
patches of a particular type (or from a particular person) we'll be in
a similar situation.
Thomas - Ben has highlighted some good points, if we don't discuss
these rationally, out in the open, we won't be able to make progress.
There are other patches from other contributors that we need to
balance the available resources against.
http://communityovercode.com/over/
Personally: my biggest concern is that we keep releasing solid high
quality code that works in production. I'm seeing patches go in that
break existing functionality and no one notices. To me the thing we
should be focusing on is adding testing. Refactoring and such to
improve/increase testing imo is a net positive.
Patrick
On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
> Jumping over to Ben's side of the discussion,
>
> Git helps with this, but does not eliminate the problem. At some point the
> changes become difficult to understand relative to the new code and may
> even collide in ways that are difficult to merge.
>
> It is true, however, that patches can be kept live using tools like git.
> That is how I kept the multi stuff alive, but there was at least one
> tricky merge to be done.
>
> On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
>
>> Thanks to Ted for replying. I will save the mail I started in the drafts
>> folder until I've calmed down again.
>>
>> Benjamin Reed:
>> > deprioritizing them doesn't help because the patches themselves bit
>> > rot. shortly they will not apply and then they will be worthless. the
>> > poor contributor would then be left with the task of maintaining a
>> > patch that would never commit.
>> You should really give GIT a try. I've kept a pipeline of half a douzend
>> patches filled and current over the last two months while drinking my
>> morning
>> coffee.
>> Likewise have I updated my development branch of over a douzend commits
>> every
>> morning against the new ZK trunk.
>>
>> Regards,
>>
>> Thomas Koch, http://www.koch.ro
>>
>
committer time available for reviews. IMO Thomas's changes have been
pretty mechanical so far, they are easy to review/commit. They do
result in some patch churn but I've been surprised it's been so little
so far and the code is improving as a result. However we've just
scratched the surface (the easy stuff), I do have some concerns as we
make more significant structural changes. Personally I think we need
to focus on beefing up testing prior to making more significant
changes.
Ben - We won't get new committers if we limit contributions. We'll
just dig a deeper hole for ourselves. Granted if we only review/accept
patches of a particular type (or from a particular person) we'll be in
a similar situation.
Thomas - Ben has highlighted some good points, if we don't discuss
these rationally, out in the open, we won't be able to make progress.
There are other patches from other contributors that we need to
balance the available resources against.
http://communityovercode.com/over/
Personally: my biggest concern is that we keep releasing solid high
quality code that works in production. I'm seeing patches go in that
break existing functionality and no one notices. To me the thing we
should be focusing on is adding testing. Refactoring and such to
improve/increase testing imo is a net positive.
Patrick
On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
> Jumping over to Ben's side of the discussion,
>
> Git helps with this, but does not eliminate the problem. At some point the
> changes become difficult to understand relative to the new code and may
> even collide in ways that are difficult to merge.
>
> It is true, however, that patches can be kept live using tools like git.
> That is how I kept the multi stuff alive, but there was at least one
> tricky merge to be done.
>
> On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
>
>> Thanks to Ted for replying. I will save the mail I started in the drafts
>> folder until I've calmed down again.
>>
>> Benjamin Reed:
>> > deprioritizing them doesn't help because the patches themselves bit
>> > rot. shortly they will not apply and then they will be worthless. the
>> > poor contributor would then be left with the task of maintaining a
>> > patch that would never commit.
>> You should really give GIT a try. I've kept a pipeline of half a douzend
>> patches filled and current over the last two months while drinking my
>> morning
>> coffee.
>> Likewise have I updated my development branch of over a douzend commits
>> every
>> morning against the new ZK trunk.
>>
>> Regards,
>>
>> Thomas Koch, http://www.koch.ro
>>
>
-
Re: cleanup and subjective patchesTed Dunning 2011-10-31, 23:08
How about a middle ground being that aesthetic changes will only be
considered if they bring significant new testing that helps mitigate the
stability risk?
On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
> EOD this is not a tool problem, it's a resource issue. We have limited
> committer time available for reviews. IMO Thomas's changes have been
> pretty mechanical so far, they are easy to review/commit. They do
> result in some patch churn but I've been surprised it's been so little
> so far and the code is improving as a result. However we've just
> scratched the surface (the easy stuff), I do have some concerns as we
> make more significant structural changes. Personally I think we need
> to focus on beefing up testing prior to making more significant
> changes.
>
> Ben - We won't get new committers if we limit contributions. We'll
> just dig a deeper hole for ourselves. Granted if we only review/accept
> patches of a particular type (or from a particular person) we'll be in
> a similar situation.
>
> Thomas - Ben has highlighted some good points, if we don't discuss
> these rationally, out in the open, we won't be able to make progress.
> There are other patches from other contributors that we need to
> balance the available resources against.
>
> http://communityovercode.com/over/
>
>
> Personally: my biggest concern is that we keep releasing solid high
> quality code that works in production. I'm seeing patches go in that
> break existing functionality and no one notices. To me the thing we
> should be focusing on is adding testing. Refactoring and such to
> improve/increase testing imo is a net positive.
>
> Patrick
>
> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]>
> wrote:
> > Jumping over to Ben's side of the discussion,
> >
> > Git helps with this, but does not eliminate the problem. At some point
> the
> > changes become difficult to understand relative to the new code and may
> > even collide in ways that are difficult to merge.
> >
> > It is true, however, that patches can be kept live using tools like git.
> > That is how I kept the multi stuff alive, but there was at least one
> > tricky merge to be done.
> >
> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
> >
> >> Thanks to Ted for replying. I will save the mail I started in the drafts
> >> folder until I've calmed down again.
> >>
> >> Benjamin Reed:
> >> > deprioritizing them doesn't help because the patches themselves bit
> >> > rot. shortly they will not apply and then they will be worthless. the
> >> > poor contributor would then be left with the task of maintaining a
> >> > patch that would never commit.
> >> You should really give GIT a try. I've kept a pipeline of half a douzend
> >> patches filled and current over the last two months while drinking my
> >> morning
> >> coffee.
> >> Likewise have I updated my development branch of over a douzend commits
> >> every
> >> morning against the new ZK trunk.
> >>
> >> Regards,
> >>
> >> Thomas Koch, http://www.koch.ro
> >>
> >
>
considered if they bring significant new testing that helps mitigate the
stability risk?
On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
> EOD this is not a tool problem, it's a resource issue. We have limited
> committer time available for reviews. IMO Thomas's changes have been
> pretty mechanical so far, they are easy to review/commit. They do
> result in some patch churn but I've been surprised it's been so little
> so far and the code is improving as a result. However we've just
> scratched the surface (the easy stuff), I do have some concerns as we
> make more significant structural changes. Personally I think we need
> to focus on beefing up testing prior to making more significant
> changes.
>
> Ben - We won't get new committers if we limit contributions. We'll
> just dig a deeper hole for ourselves. Granted if we only review/accept
> patches of a particular type (or from a particular person) we'll be in
> a similar situation.
>
> Thomas - Ben has highlighted some good points, if we don't discuss
> these rationally, out in the open, we won't be able to make progress.
> There are other patches from other contributors that we need to
> balance the available resources against.
>
> http://communityovercode.com/over/
>
>
> Personally: my biggest concern is that we keep releasing solid high
> quality code that works in production. I'm seeing patches go in that
> break existing functionality and no one notices. To me the thing we
> should be focusing on is adding testing. Refactoring and such to
> improve/increase testing imo is a net positive.
>
> Patrick
>
> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]>
> wrote:
> > Jumping over to Ben's side of the discussion,
> >
> > Git helps with this, but does not eliminate the problem. At some point
> the
> > changes become difficult to understand relative to the new code and may
> > even collide in ways that are difficult to merge.
> >
> > It is true, however, that patches can be kept live using tools like git.
> > That is how I kept the multi stuff alive, but there was at least one
> > tricky merge to be done.
> >
> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
> >
> >> Thanks to Ted for replying. I will save the mail I started in the drafts
> >> folder until I've calmed down again.
> >>
> >> Benjamin Reed:
> >> > deprioritizing them doesn't help because the patches themselves bit
> >> > rot. shortly they will not apply and then they will be worthless. the
> >> > poor contributor would then be left with the task of maintaining a
> >> > patch that would never commit.
> >> You should really give GIT a try. I've kept a pipeline of half a douzend
> >> patches filled and current over the last two months while drinking my
> >> morning
> >> coffee.
> >> Likewise have I updated my development branch of over a douzend commits
> >> every
> >> morning against the new ZK trunk.
> >>
> >> Regards,
> >>
> >> Thomas Koch, http://www.koch.ro
> >>
> >
>
-
Re: cleanup and subjective patchesBenjamin Reed 2011-10-31, 23:28
that isn't really middle ground. adding new tests for code is added
functionality, so it would be a valid patch under the criteria i
suggested. changing an int to an enum is not adding functionality. the
later may be subjectively better and the former adds functionality.
the nice thing is that you can determine that objectively. :)
ben
On Mon, Oct 31, 2011 at 4:08 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
> How about a middle ground being that aesthetic changes will only be
> considered if they bring significant new testing that helps mitigate the
> stability risk?
>
> On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
>>
>> EOD this is not a tool problem, it's a resource issue. We have limited
>> committer time available for reviews. IMO Thomas's changes have been
>> pretty mechanical so far, they are easy to review/commit. They do
>> result in some patch churn but I've been surprised it's been so little
>> so far and the code is improving as a result. However we've just
>> scratched the surface (the easy stuff), I do have some concerns as we
>> make more significant structural changes. Personally I think we need
>> to focus on beefing up testing prior to making more significant
>> changes.
>>
>> Ben - We won't get new committers if we limit contributions. We'll
>> just dig a deeper hole for ourselves. Granted if we only review/accept
>> patches of a particular type (or from a particular person) we'll be in
>> a similar situation.
>>
>> Thomas - Ben has highlighted some good points, if we don't discuss
>> these rationally, out in the open, we won't be able to make progress.
>> There are other patches from other contributors that we need to
>> balance the available resources against.
>>
>> http://communityovercode.com/over/
>>
>>
>> Personally: my biggest concern is that we keep releasing solid high
>> quality code that works in production. I'm seeing patches go in that
>> break existing functionality and no one notices. To me the thing we
>> should be focusing on is adding testing. Refactoring and such to
>> improve/increase testing imo is a net positive.
>>
>> Patrick
>>
>> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]>
>> wrote:
>> > Jumping over to Ben's side of the discussion,
>> >
>> > Git helps with this, but does not eliminate the problem. At some point
>> > the
>> > changes become difficult to understand relative to the new code and may
>> > even collide in ways that are difficult to merge.
>> >
>> > It is true, however, that patches can be kept live using tools like git.
>> > That is how I kept the multi stuff alive, but there was at least one
>> > tricky merge to be done.
>> >
>> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
>> >
>> >> Thanks to Ted for replying. I will save the mail I started in the
>> >> drafts
>> >> folder until I've calmed down again.
>> >>
>> >> Benjamin Reed:
>> >> > deprioritizing them doesn't help because the patches themselves bit
>> >> > rot. shortly they will not apply and then they will be worthless. the
>> >> > poor contributor would then be left with the task of maintaining a
>> >> > patch that would never commit.
>> >> You should really give GIT a try. I've kept a pipeline of half a
>> >> douzend
>> >> patches filled and current over the last two months while drinking my
>> >> morning
>> >> coffee.
>> >> Likewise have I updated my development branch of over a douzend commits
>> >> every
>> >> morning against the new ZK trunk.
>> >>
>> >> Regards,
>> >>
>> >> Thomas Koch, http://www.koch.ro
>> >>
>> >
>
>
functionality, so it would be a valid patch under the criteria i
suggested. changing an int to an enum is not adding functionality. the
later may be subjectively better and the former adds functionality.
the nice thing is that you can determine that objectively. :)
ben
On Mon, Oct 31, 2011 at 4:08 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
> How about a middle ground being that aesthetic changes will only be
> considered if they bring significant new testing that helps mitigate the
> stability risk?
>
> On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
>>
>> EOD this is not a tool problem, it's a resource issue. We have limited
>> committer time available for reviews. IMO Thomas's changes have been
>> pretty mechanical so far, they are easy to review/commit. They do
>> result in some patch churn but I've been surprised it's been so little
>> so far and the code is improving as a result. However we've just
>> scratched the surface (the easy stuff), I do have some concerns as we
>> make more significant structural changes. Personally I think we need
>> to focus on beefing up testing prior to making more significant
>> changes.
>>
>> Ben - We won't get new committers if we limit contributions. We'll
>> just dig a deeper hole for ourselves. Granted if we only review/accept
>> patches of a particular type (or from a particular person) we'll be in
>> a similar situation.
>>
>> Thomas - Ben has highlighted some good points, if we don't discuss
>> these rationally, out in the open, we won't be able to make progress.
>> There are other patches from other contributors that we need to
>> balance the available resources against.
>>
>> http://communityovercode.com/over/
>>
>>
>> Personally: my biggest concern is that we keep releasing solid high
>> quality code that works in production. I'm seeing patches go in that
>> break existing functionality and no one notices. To me the thing we
>> should be focusing on is adding testing. Refactoring and such to
>> improve/increase testing imo is a net positive.
>>
>> Patrick
>>
>> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]>
>> wrote:
>> > Jumping over to Ben's side of the discussion,
>> >
>> > Git helps with this, but does not eliminate the problem. At some point
>> > the
>> > changes become difficult to understand relative to the new code and may
>> > even collide in ways that are difficult to merge.
>> >
>> > It is true, however, that patches can be kept live using tools like git.
>> > That is how I kept the multi stuff alive, but there was at least one
>> > tricky merge to be done.
>> >
>> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
>> >
>> >> Thanks to Ted for replying. I will save the mail I started in the
>> >> drafts
>> >> folder until I've calmed down again.
>> >>
>> >> Benjamin Reed:
>> >> > deprioritizing them doesn't help because the patches themselves bit
>> >> > rot. shortly they will not apply and then they will be worthless. the
>> >> > poor contributor would then be left with the task of maintaining a
>> >> > patch that would never commit.
>> >> You should really give GIT a try. I've kept a pipeline of half a
>> >> douzend
>> >> patches filled and current over the last two months while drinking my
>> >> morning
>> >> coffee.
>> >> Likewise have I updated my development branch of over a douzend commits
>> >> every
>> >> morning against the new ZK trunk.
>> >>
>> >> Regards,
>> >>
>> >> Thomas Koch, http://www.koch.ro
>> >>
>> >
>
>
-
Re: cleanup and subjective patchesTed Dunning 2011-10-31, 23:30
But would a combined test + aesthetic patch fly in your mind?
On Mon, Oct 31, 2011 at 4:28 PM, Benjamin Reed <[EMAIL PROTECTED]> wrote:
> that isn't really middle ground. adding new tests for code is added
> functionality, so it would be a valid patch under the criteria i
> suggested. changing an int to an enum is not adding functionality. the
> later may be subjectively better and the former adds functionality.
> the nice thing is that you can determine that objectively. :)
>
> ben
>
> On Mon, Oct 31, 2011 at 4:08 PM, Ted Dunning <[EMAIL PROTECTED]>
> wrote:
> > How about a middle ground being that aesthetic changes will only be
> > considered if they bring significant new testing that helps mitigate the
> > stability risk?
> >
> > On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
> >>
> >> EOD this is not a tool problem, it's a resource issue. We have limited
> >> committer time available for reviews. IMO Thomas's changes have been
> >> pretty mechanical so far, they are easy to review/commit. They do
> >> result in some patch churn but I've been surprised it's been so little
> >> so far and the code is improving as a result. However we've just
> >> scratched the surface (the easy stuff), I do have some concerns as we
> >> make more significant structural changes. Personally I think we need
> >> to focus on beefing up testing prior to making more significant
> >> changes.
> >>
> >> Ben - We won't get new committers if we limit contributions. We'll
> >> just dig a deeper hole for ourselves. Granted if we only review/accept
> >> patches of a particular type (or from a particular person) we'll be in
> >> a similar situation.
> >>
> >> Thomas - Ben has highlighted some good points, if we don't discuss
> >> these rationally, out in the open, we won't be able to make progress.
> >> There are other patches from other contributors that we need to
> >> balance the available resources against.
> >>
> >> http://communityovercode.com/over/
> >>
> >>
> >> Personally: my biggest concern is that we keep releasing solid high
> >> quality code that works in production. I'm seeing patches go in that
> >> break existing functionality and no one notices. To me the thing we
> >> should be focusing on is adding testing. Refactoring and such to
> >> improve/increase testing imo is a net positive.
> >>
> >> Patrick
> >>
> >> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]>
> >> wrote:
> >> > Jumping over to Ben's side of the discussion,
> >> >
> >> > Git helps with this, but does not eliminate the problem. At some
> point
> >> > the
> >> > changes become difficult to understand relative to the new code and
> may
> >> > even collide in ways that are difficult to merge.
> >> >
> >> > It is true, however, that patches can be kept live using tools like
> git.
> >> > That is how I kept the multi stuff alive, but there was at least one
> >> > tricky merge to be done.
> >> >
> >> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
> >> >
> >> >> Thanks to Ted for replying. I will save the mail I started in the
> >> >> drafts
> >> >> folder until I've calmed down again.
> >> >>
> >> >> Benjamin Reed:
> >> >> > deprioritizing them doesn't help because the patches themselves bit
> >> >> > rot. shortly they will not apply and then they will be worthless.
> the
> >> >> > poor contributor would then be left with the task of maintaining a
> >> >> > patch that would never commit.
> >> >> You should really give GIT a try. I've kept a pipeline of half a
> >> >> douzend
> >> >> patches filled and current over the last two months while drinking my
> >> >> morning
> >> >> coffee.
> >> >> Likewise have I updated my development branch of over a douzend
> commits
> >> >> every
> >> >> morning against the new ZK trunk.
> >> >>
> >> >> Regards,
> >> >>
> >> >> Thomas Koch, http://www.koch.ro
> >> >>
> >> >
> >
> >
>
On Mon, Oct 31, 2011 at 4:28 PM, Benjamin Reed <[EMAIL PROTECTED]> wrote:
> that isn't really middle ground. adding new tests for code is added
> functionality, so it would be a valid patch under the criteria i
> suggested. changing an int to an enum is not adding functionality. the
> later may be subjectively better and the former adds functionality.
> the nice thing is that you can determine that objectively. :)
>
> ben
>
> On Mon, Oct 31, 2011 at 4:08 PM, Ted Dunning <[EMAIL PROTECTED]>
> wrote:
> > How about a middle ground being that aesthetic changes will only be
> > considered if they bring significant new testing that helps mitigate the
> > stability risk?
> >
> > On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
> >>
> >> EOD this is not a tool problem, it's a resource issue. We have limited
> >> committer time available for reviews. IMO Thomas's changes have been
> >> pretty mechanical so far, they are easy to review/commit. They do
> >> result in some patch churn but I've been surprised it's been so little
> >> so far and the code is improving as a result. However we've just
> >> scratched the surface (the easy stuff), I do have some concerns as we
> >> make more significant structural changes. Personally I think we need
> >> to focus on beefing up testing prior to making more significant
> >> changes.
> >>
> >> Ben - We won't get new committers if we limit contributions. We'll
> >> just dig a deeper hole for ourselves. Granted if we only review/accept
> >> patches of a particular type (or from a particular person) we'll be in
> >> a similar situation.
> >>
> >> Thomas - Ben has highlighted some good points, if we don't discuss
> >> these rationally, out in the open, we won't be able to make progress.
> >> There are other patches from other contributors that we need to
> >> balance the available resources against.
> >>
> >> http://communityovercode.com/over/
> >>
> >>
> >> Personally: my biggest concern is that we keep releasing solid high
> >> quality code that works in production. I'm seeing patches go in that
> >> break existing functionality and no one notices. To me the thing we
> >> should be focusing on is adding testing. Refactoring and such to
> >> improve/increase testing imo is a net positive.
> >>
> >> Patrick
> >>
> >> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]>
> >> wrote:
> >> > Jumping over to Ben's side of the discussion,
> >> >
> >> > Git helps with this, but does not eliminate the problem. At some
> point
> >> > the
> >> > changes become difficult to understand relative to the new code and
> may
> >> > even collide in ways that are difficult to merge.
> >> >
> >> > It is true, however, that patches can be kept live using tools like
> git.
> >> > That is how I kept the multi stuff alive, but there was at least one
> >> > tricky merge to be done.
> >> >
> >> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
> >> >
> >> >> Thanks to Ted for replying. I will save the mail I started in the
> >> >> drafts
> >> >> folder until I've calmed down again.
> >> >>
> >> >> Benjamin Reed:
> >> >> > deprioritizing them doesn't help because the patches themselves bit
> >> >> > rot. shortly they will not apply and then they will be worthless.
> the
> >> >> > poor contributor would then be left with the task of maintaining a
> >> >> > patch that would never commit.
> >> >> You should really give GIT a try. I've kept a pipeline of half a
> >> >> douzend
> >> >> patches filled and current over the last two months while drinking my
> >> >> morning
> >> >> coffee.
> >> >> Likewise have I updated my development branch of over a douzend
> commits
> >> >> every
> >> >> morning against the new ZK trunk.
> >> >>
> >> >> Regards,
> >> >>
> >> >> Thomas Koch, http://www.koch.ro
> >> >>
> >> >
> >
> >
>
-
Re: cleanup and subjective patchesPatrick Hunt 2011-10-31, 23:31
What is an aesthetic change? Thomas just removed a duplicate test in
the source base (copy/paste), should that go in or not? (imo it
should) Many of the changes that have been going in are improvements,
such as reducing warnings in eclipse so that when you are using
eclipse you can actually see "real" warnings. etc.... I've pushed back
on a few patches that were more aesthetic imo, but not many fell into
that category. Who knows if my sense of style is the same as yours...
etc...
The problem with demanding more tests is that we need to improve the
test framework itself for that to be successful. Right now most of our
tests are not unit tests. It's hard to test at the unit level. With
each release I've tried to refactor/improve the test framework, but
it's still not up to par.
I'd love it if someone were willing to work with me on the testing.
Thomas? That would provide a strong foundation for future
refactorings.
Patrick
On Mon, Oct 31, 2011 at 4:08 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
> How about a middle ground being that aesthetic changes will only be
> considered if they bring significant new testing that helps mitigate the
> stability risk?
>
> On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
>
>> EOD this is not a tool problem, it's a resource issue. We have limited
>> committer time available for reviews. IMO Thomas's changes have been
>> pretty mechanical so far, they are easy to review/commit. They do
>> result in some patch churn but I've been surprised it's been so little
>> so far and the code is improving as a result. However we've just
>> scratched the surface (the easy stuff), I do have some concerns as we
>> make more significant structural changes. Personally I think we need
>> to focus on beefing up testing prior to making more significant
>> changes.
>>
>> Ben - We won't get new committers if we limit contributions. We'll
>> just dig a deeper hole for ourselves. Granted if we only review/accept
>> patches of a particular type (or from a particular person) we'll be in
>> a similar situation.
>>
>> Thomas - Ben has highlighted some good points, if we don't discuss
>> these rationally, out in the open, we won't be able to make progress.
>> There are other patches from other contributors that we need to
>> balance the available resources against.
>>
>> http://communityovercode.com/over/
>>
>>
>> Personally: my biggest concern is that we keep releasing solid high
>> quality code that works in production. I'm seeing patches go in that
>> break existing functionality and no one notices. To me the thing we
>> should be focusing on is adding testing. Refactoring and such to
>> improve/increase testing imo is a net positive.
>>
>> Patrick
>>
>> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]>
>> wrote:
>> > Jumping over to Ben's side of the discussion,
>> >
>> > Git helps with this, but does not eliminate the problem. At some point
>> the
>> > changes become difficult to understand relative to the new code and may
>> > even collide in ways that are difficult to merge.
>> >
>> > It is true, however, that patches can be kept live using tools like git.
>> > That is how I kept the multi stuff alive, but there was at least one
>> > tricky merge to be done.
>> >
>> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
>> >
>> >> Thanks to Ted for replying. I will save the mail I started in the drafts
>> >> folder until I've calmed down again.
>> >>
>> >> Benjamin Reed:
>> >> > deprioritizing them doesn't help because the patches themselves bit
>> >> > rot. shortly they will not apply and then they will be worthless. the
>> >> > poor contributor would then be left with the task of maintaining a
>> >> > patch that would never commit.
>> >> You should really give GIT a try. I've kept a pipeline of half a douzend
>> >> patches filled and current over the last two months while drinking my
>> >> morning
>> >> coffee.
>> >> Likewise have I updated my development branch of over a douzend commits
the source base (copy/paste), should that go in or not? (imo it
should) Many of the changes that have been going in are improvements,
such as reducing warnings in eclipse so that when you are using
eclipse you can actually see "real" warnings. etc.... I've pushed back
on a few patches that were more aesthetic imo, but not many fell into
that category. Who knows if my sense of style is the same as yours...
etc...
The problem with demanding more tests is that we need to improve the
test framework itself for that to be successful. Right now most of our
tests are not unit tests. It's hard to test at the unit level. With
each release I've tried to refactor/improve the test framework, but
it's still not up to par.
I'd love it if someone were willing to work with me on the testing.
Thomas? That would provide a strong foundation for future
refactorings.
Patrick
On Mon, Oct 31, 2011 at 4:08 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
> How about a middle ground being that aesthetic changes will only be
> considered if they bring significant new testing that helps mitigate the
> stability risk?
>
> On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
>
>> EOD this is not a tool problem, it's a resource issue. We have limited
>> committer time available for reviews. IMO Thomas's changes have been
>> pretty mechanical so far, they are easy to review/commit. They do
>> result in some patch churn but I've been surprised it's been so little
>> so far and the code is improving as a result. However we've just
>> scratched the surface (the easy stuff), I do have some concerns as we
>> make more significant structural changes. Personally I think we need
>> to focus on beefing up testing prior to making more significant
>> changes.
>>
>> Ben - We won't get new committers if we limit contributions. We'll
>> just dig a deeper hole for ourselves. Granted if we only review/accept
>> patches of a particular type (or from a particular person) we'll be in
>> a similar situation.
>>
>> Thomas - Ben has highlighted some good points, if we don't discuss
>> these rationally, out in the open, we won't be able to make progress.
>> There are other patches from other contributors that we need to
>> balance the available resources against.
>>
>> http://communityovercode.com/over/
>>
>>
>> Personally: my biggest concern is that we keep releasing solid high
>> quality code that works in production. I'm seeing patches go in that
>> break existing functionality and no one notices. To me the thing we
>> should be focusing on is adding testing. Refactoring and such to
>> improve/increase testing imo is a net positive.
>>
>> Patrick
>>
>> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]>
>> wrote:
>> > Jumping over to Ben's side of the discussion,
>> >
>> > Git helps with this, but does not eliminate the problem. At some point
>> the
>> > changes become difficult to understand relative to the new code and may
>> > even collide in ways that are difficult to merge.
>> >
>> > It is true, however, that patches can be kept live using tools like git.
>> > That is how I kept the multi stuff alive, but there was at least one
>> > tricky merge to be done.
>> >
>> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
>> >
>> >> Thanks to Ted for replying. I will save the mail I started in the drafts
>> >> folder until I've calmed down again.
>> >>
>> >> Benjamin Reed:
>> >> > deprioritizing them doesn't help because the patches themselves bit
>> >> > rot. shortly they will not apply and then they will be worthless. the
>> >> > poor contributor would then be left with the task of maintaining a
>> >> > patch that would never commit.
>> >> You should really give GIT a try. I've kept a pipeline of half a douzend
>> >> patches filled and current over the last two months while drinking my
>> >> morning
>> >> coffee.
>> >> Likewise have I updated my development branch of over a douzend commits
-
Re: cleanup and subjective patchesTed Dunning 2011-10-31, 23:43
Thomas,
Let me second that suggestion. If you could figure out how to make tests
easier to do without starting a full scale server, it would make a BIG
positive impact.
On Mon, Oct 31, 2011 at 4:31 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
> I'd love it if someone were willing to work with me on the testing.
> Thomas? That would provide a strong foundation for future
> refactorings.
>
Let me second that suggestion. If you could figure out how to make tests
easier to do without starting a full scale server, it would make a BIG
positive impact.
On Mon, Oct 31, 2011 at 4:31 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
> I'd love it if someone were willing to work with me on the testing.
> Thomas? That would provide a strong foundation for future
> refactorings.
>
-
Re: cleanup and subjective patchesMahadev Konar 2011-11-01, 01:47
I'd have to agree with Pat here. I think we should be open to code
cleanup especially code cleanup that makes testing easier. There is no
harm in folks contributing minor cleanups (all the more easier to
review). It is a nice way of encouraging folks to contribute more and
get more involved in the community.
I'd personally like some cleanup in the code especially -
1) NIO code
2) Leader Election code
3) QuorumPeer/Leader/Follower/ZooKeeperServer interaction.
This would make testing easier and would make the code maintainable in
the longer run.
thanks
mahadev
On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
> EOD this is not a tool problem, it's a resource issue. We have limited
> committer time available for reviews. IMO Thomas's changes have been
> pretty mechanical so far, they are easy to review/commit. They do
> result in some patch churn but I've been surprised it's been so little
> so far and the code is improving as a result. However we've just
> scratched the surface (the easy stuff), I do have some concerns as we
> make more significant structural changes. Personally I think we need
> to focus on beefing up testing prior to making more significant
> changes.
>
> Ben - We won't get new committers if we limit contributions. We'll
> just dig a deeper hole for ourselves. Granted if we only review/accept
> patches of a particular type (or from a particular person) we'll be in
> a similar situation.
>
> Thomas - Ben has highlighted some good points, if we don't discuss
> these rationally, out in the open, we won't be able to make progress.
> There are other patches from other contributors that we need to
> balance the available resources against.
>
> http://communityovercode.com/over/
>
>
> Personally: my biggest concern is that we keep releasing solid high
> quality code that works in production. I'm seeing patches go in that
> break existing functionality and no one notices. To me the thing we
> should be focusing on is adding testing. Refactoring and such to
> improve/increase testing imo is a net positive.
>
> Patrick
>
> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
>> Jumping over to Ben's side of the discussion,
>>
>> Git helps with this, but does not eliminate the problem. At some point the
>> changes become difficult to understand relative to the new code and may
>> even collide in ways that are difficult to merge.
>>
>> It is true, however, that patches can be kept live using tools like git.
>> That is how I kept the multi stuff alive, but there was at least one
>> tricky merge to be done.
>>
>> On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
>>
>>> Thanks to Ted for replying. I will save the mail I started in the drafts
>>> folder until I've calmed down again.
>>>
>>> Benjamin Reed:
>>> > deprioritizing them doesn't help because the patches themselves bit
>>> > rot. shortly they will not apply and then they will be worthless. the
>>> > poor contributor would then be left with the task of maintaining a
>>> > patch that would never commit.
>>> You should really give GIT a try. I've kept a pipeline of half a douzend
>>> patches filled and current over the last two months while drinking my
>>> morning
>>> coffee.
>>> Likewise have I updated my development branch of over a douzend commits
>>> every
>>> morning against the new ZK trunk.
>>>
>>> Regards,
>>>
>>> Thomas Koch, http://www.koch.ro
>>>
>>
>
cleanup especially code cleanup that makes testing easier. There is no
harm in folks contributing minor cleanups (all the more easier to
review). It is a nice way of encouraging folks to contribute more and
get more involved in the community.
I'd personally like some cleanup in the code especially -
1) NIO code
2) Leader Election code
3) QuorumPeer/Leader/Follower/ZooKeeperServer interaction.
This would make testing easier and would make the code maintainable in
the longer run.
thanks
mahadev
On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
> EOD this is not a tool problem, it's a resource issue. We have limited
> committer time available for reviews. IMO Thomas's changes have been
> pretty mechanical so far, they are easy to review/commit. They do
> result in some patch churn but I've been surprised it's been so little
> so far and the code is improving as a result. However we've just
> scratched the surface (the easy stuff), I do have some concerns as we
> make more significant structural changes. Personally I think we need
> to focus on beefing up testing prior to making more significant
> changes.
>
> Ben - We won't get new committers if we limit contributions. We'll
> just dig a deeper hole for ourselves. Granted if we only review/accept
> patches of a particular type (or from a particular person) we'll be in
> a similar situation.
>
> Thomas - Ben has highlighted some good points, if we don't discuss
> these rationally, out in the open, we won't be able to make progress.
> There are other patches from other contributors that we need to
> balance the available resources against.
>
> http://communityovercode.com/over/
>
>
> Personally: my biggest concern is that we keep releasing solid high
> quality code that works in production. I'm seeing patches go in that
> break existing functionality and no one notices. To me the thing we
> should be focusing on is adding testing. Refactoring and such to
> improve/increase testing imo is a net positive.
>
> Patrick
>
> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
>> Jumping over to Ben's side of the discussion,
>>
>> Git helps with this, but does not eliminate the problem. At some point the
>> changes become difficult to understand relative to the new code and may
>> even collide in ways that are difficult to merge.
>>
>> It is true, however, that patches can be kept live using tools like git.
>> That is how I kept the multi stuff alive, but there was at least one
>> tricky merge to be done.
>>
>> On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
>>
>>> Thanks to Ted for replying. I will save the mail I started in the drafts
>>> folder until I've calmed down again.
>>>
>>> Benjamin Reed:
>>> > deprioritizing them doesn't help because the patches themselves bit
>>> > rot. shortly they will not apply and then they will be worthless. the
>>> > poor contributor would then be left with the task of maintaining a
>>> > patch that would never commit.
>>> You should really give GIT a try. I've kept a pipeline of half a douzend
>>> patches filled and current over the last two months while drinking my
>>> morning
>>> coffee.
>>> Likewise have I updated my development branch of over a douzend commits
>>> every
>>> morning against the new ZK trunk.
>>>
>>> Regards,
>>>
>>> Thomas Koch, http://www.koch.ro
>>>
>>
>
-
Re: cleanup and subjective patchesBenjamin Reed 2011-11-01, 04:39
those pieces of code are core to the system. as i said for
reasonsearlier, risking destabilizing them to make the code look
prettier forsome definition of pretty does not seem wise to me.
changing code toadd functionality such as testability, performance,
bug fix, etc doesseem wise.
even then, i tend to be conservative. when i did the protocol fix,
ireally wanted to clean up some things, but i knew if i cleaned up
andfixed the protocol at the same time i both risked introducing a
bugand it made it harder to see exactly what i was changing
forreviewers. i figured it would be better to wait to do a cleanup
whenwe put in a real unit test so that when the refactoring happens we
canknow that it is useful.
i'm fine with losing this argument, but if you guys really believethat
i'm wrong please start committing the cleanup patches! i'm notgoing
to, but i'm fine if another committer takes responsibility forit.
either way, there are some rather big patches out there that
arerotting and may conflict with current and future patches so
decisionsneed to be made.
ben
On Mon, Oct 31, 2011 at 6:47 PM, Mahadev Konar <[EMAIL PROTECTED]> wrote:
> I'd have to agree with Pat here. I think we should be open to code
> cleanup especially code cleanup that makes testing easier. There is no
> harm in folks contributing minor cleanups (all the more easier to
> review). It is a nice way of encouraging folks to contribute more and
> get more involved in the community.
>
> I'd personally like some cleanup in the code especially -
>
> 1) NIO code
> 2) Leader Election code
> 3) QuorumPeer/Leader/Follower/ZooKeeperServer interaction.
>
> This would make testing easier and would make the code maintainable in
> the longer run.
>
> thanks
> mahadev
>
> On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
>> EOD this is not a tool problem, it's a resource issue. We have limited
>> committer time available for reviews. IMO Thomas's changes have been
>> pretty mechanical so far, they are easy to review/commit. They do
>> result in some patch churn but I've been surprised it's been so little
>> so far and the code is improving as a result. However we've just
>> scratched the surface (the easy stuff), I do have some concerns as we
>> make more significant structural changes. Personally I think we need
>> to focus on beefing up testing prior to making more significant
>> changes.
>>
>> Ben - We won't get new committers if we limit contributions. We'll
>> just dig a deeper hole for ourselves. Granted if we only review/accept
>> patches of a particular type (or from a particular person) we'll be in
>> a similar situation.
>>
>> Thomas - Ben has highlighted some good points, if we don't discuss
>> these rationally, out in the open, we won't be able to make progress.
>> There are other patches from other contributors that we need to
>> balance the available resources against.
>>
>> http://communityovercode.com/over/
>>
>>
>> Personally: my biggest concern is that we keep releasing solid high
>> quality code that works in production. I'm seeing patches go in that
>> break existing functionality and no one notices. To me the thing we
>> should be focusing on is adding testing. Refactoring and such to
>> improve/increase testing imo is a net positive.
>>
>> Patrick
>>
>> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
>>> Jumping over to Ben's side of the discussion,
>>>
>>> Git helps with this, but does not eliminate the problem. At some point the
>>> changes become difficult to understand relative to the new code and may
>>> even collide in ways that are difficult to merge.
>>>
>>> It is true, however, that patches can be kept live using tools like git.
>>> That is how I kept the multi stuff alive, but there was at least one
>>> tricky merge to be done.
>>>
>>> On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
>>>
>>>> Thanks to Ted for replying. I will save the mail I started in the drafts
>>>> folder until I've calmed down again.
reasonsearlier, risking destabilizing them to make the code look
prettier forsome definition of pretty does not seem wise to me.
changing code toadd functionality such as testability, performance,
bug fix, etc doesseem wise.
even then, i tend to be conservative. when i did the protocol fix,
ireally wanted to clean up some things, but i knew if i cleaned up
andfixed the protocol at the same time i both risked introducing a
bugand it made it harder to see exactly what i was changing
forreviewers. i figured it would be better to wait to do a cleanup
whenwe put in a real unit test so that when the refactoring happens we
canknow that it is useful.
i'm fine with losing this argument, but if you guys really believethat
i'm wrong please start committing the cleanup patches! i'm notgoing
to, but i'm fine if another committer takes responsibility forit.
either way, there are some rather big patches out there that
arerotting and may conflict with current and future patches so
decisionsneed to be made.
ben
On Mon, Oct 31, 2011 at 6:47 PM, Mahadev Konar <[EMAIL PROTECTED]> wrote:
> I'd have to agree with Pat here. I think we should be open to code
> cleanup especially code cleanup that makes testing easier. There is no
> harm in folks contributing minor cleanups (all the more easier to
> review). It is a nice way of encouraging folks to contribute more and
> get more involved in the community.
>
> I'd personally like some cleanup in the code especially -
>
> 1) NIO code
> 2) Leader Election code
> 3) QuorumPeer/Leader/Follower/ZooKeeperServer interaction.
>
> This would make testing easier and would make the code maintainable in
> the longer run.
>
> thanks
> mahadev
>
> On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
>> EOD this is not a tool problem, it's a resource issue. We have limited
>> committer time available for reviews. IMO Thomas's changes have been
>> pretty mechanical so far, they are easy to review/commit. They do
>> result in some patch churn but I've been surprised it's been so little
>> so far and the code is improving as a result. However we've just
>> scratched the surface (the easy stuff), I do have some concerns as we
>> make more significant structural changes. Personally I think we need
>> to focus on beefing up testing prior to making more significant
>> changes.
>>
>> Ben - We won't get new committers if we limit contributions. We'll
>> just dig a deeper hole for ourselves. Granted if we only review/accept
>> patches of a particular type (or from a particular person) we'll be in
>> a similar situation.
>>
>> Thomas - Ben has highlighted some good points, if we don't discuss
>> these rationally, out in the open, we won't be able to make progress.
>> There are other patches from other contributors that we need to
>> balance the available resources against.
>>
>> http://communityovercode.com/over/
>>
>>
>> Personally: my biggest concern is that we keep releasing solid high
>> quality code that works in production. I'm seeing patches go in that
>> break existing functionality and no one notices. To me the thing we
>> should be focusing on is adding testing. Refactoring and such to
>> improve/increase testing imo is a net positive.
>>
>> Patrick
>>
>> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
>>> Jumping over to Ben's side of the discussion,
>>>
>>> Git helps with this, but does not eliminate the problem. At some point the
>>> changes become difficult to understand relative to the new code and may
>>> even collide in ways that are difficult to merge.
>>>
>>> It is true, however, that patches can be kept live using tools like git.
>>> That is how I kept the multi stuff alive, but there was at least one
>>> tricky merge to be done.
>>>
>>> On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
>>>
>>>> Thanks to Ted for replying. I will save the mail I started in the drafts
>>>> folder until I've calmed down again.
-
RE: cleanup and subjective patchesFournier, Camille F. 2011-11-01, 14:21
Any changes coming in without tests should really be meaningfully untestable. I completely agree with the suggestion to require a testing uplift if you want to add refactorings unless you know the refactored code has 90+% test coverage.
Personally, I have no problems with refactorings, but we seem to be spending much of our time dealing with them right now when we really need to get 3.4 out the door. It's been months that this release has been going on and splitting the committer attention between 3.4 and refactoring changes feels counterproductive to the needs of the community at this moment.
C
-----Original Message-----
From: Ted Dunning [mailto:[EMAIL PROTECTED]]
Sent: Monday, October 31, 2011 7:08 PM
To: [EMAIL PROTECTED]
Cc: [EMAIL PROTECTED]; Benjamin Reed
Subject: Re: cleanup and subjective patches
How about a middle ground being that aesthetic changes will only be
considered if they bring significant new testing that helps mitigate the
stability risk?
On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
> EOD this is not a tool problem, it's a resource issue. We have limited
> committer time available for reviews. IMO Thomas's changes have been
> pretty mechanical so far, they are easy to review/commit. They do
> result in some patch churn but I've been surprised it's been so little
> so far and the code is improving as a result. However we've just
> scratched the surface (the easy stuff), I do have some concerns as we
> make more significant structural changes. Personally I think we need
> to focus on beefing up testing prior to making more significant
> changes.
>
> Ben - We won't get new committers if we limit contributions. We'll
> just dig a deeper hole for ourselves. Granted if we only review/accept
> patches of a particular type (or from a particular person) we'll be in
> a similar situation.
>
> Thomas - Ben has highlighted some good points, if we don't discuss
> these rationally, out in the open, we won't be able to make progress.
> There are other patches from other contributors that we need to
> balance the available resources against.
>
> http://communityovercode.com/over/
>
>
> Personally: my biggest concern is that we keep releasing solid high
> quality code that works in production. I'm seeing patches go in that
> break existing functionality and no one notices. To me the thing we
> should be focusing on is adding testing. Refactoring and such to
> improve/increase testing imo is a net positive.
>
> Patrick
>
> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]>
> wrote:
> > Jumping over to Ben's side of the discussion,
> >
> > Git helps with this, but does not eliminate the problem. At some point
> the
> > changes become difficult to understand relative to the new code and may
> > even collide in ways that are difficult to merge.
> >
> > It is true, however, that patches can be kept live using tools like git.
> > That is how I kept the multi stuff alive, but there was at least one
> > tricky merge to be done.
> >
> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
> >
> >> Thanks to Ted for replying. I will save the mail I started in the drafts
> >> folder until I've calmed down again.
> >>
> >> Benjamin Reed:
> >> > deprioritizing them doesn't help because the patches themselves bit
> >> > rot. shortly they will not apply and then they will be worthless. the
> >> > poor contributor would then be left with the task of maintaining a
> >> > patch that would never commit.
> >> You should really give GIT a try. I've kept a pipeline of half a douzend
> >> patches filled and current over the last two months while drinking my
> >> morning
> >> coffee.
> >> Likewise have I updated my development branch of over a douzend commits
> >> every
> >> morning against the new ZK trunk.
> >>
> >> Regards,
> >>
> >> Thomas Koch, http://www.koch.ro
> >>
> >
>
Personally, I have no problems with refactorings, but we seem to be spending much of our time dealing with them right now when we really need to get 3.4 out the door. It's been months that this release has been going on and splitting the committer attention between 3.4 and refactoring changes feels counterproductive to the needs of the community at this moment.
C
-----Original Message-----
From: Ted Dunning [mailto:[EMAIL PROTECTED]]
Sent: Monday, October 31, 2011 7:08 PM
To: [EMAIL PROTECTED]
Cc: [EMAIL PROTECTED]; Benjamin Reed
Subject: Re: cleanup and subjective patches
How about a middle ground being that aesthetic changes will only be
considered if they bring significant new testing that helps mitigate the
stability risk?
On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
> EOD this is not a tool problem, it's a resource issue. We have limited
> committer time available for reviews. IMO Thomas's changes have been
> pretty mechanical so far, they are easy to review/commit. They do
> result in some patch churn but I've been surprised it's been so little
> so far and the code is improving as a result. However we've just
> scratched the surface (the easy stuff), I do have some concerns as we
> make more significant structural changes. Personally I think we need
> to focus on beefing up testing prior to making more significant
> changes.
>
> Ben - We won't get new committers if we limit contributions. We'll
> just dig a deeper hole for ourselves. Granted if we only review/accept
> patches of a particular type (or from a particular person) we'll be in
> a similar situation.
>
> Thomas - Ben has highlighted some good points, if we don't discuss
> these rationally, out in the open, we won't be able to make progress.
> There are other patches from other contributors that we need to
> balance the available resources against.
>
> http://communityovercode.com/over/
>
>
> Personally: my biggest concern is that we keep releasing solid high
> quality code that works in production. I'm seeing patches go in that
> break existing functionality and no one notices. To me the thing we
> should be focusing on is adding testing. Refactoring and such to
> improve/increase testing imo is a net positive.
>
> Patrick
>
> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]>
> wrote:
> > Jumping over to Ben's side of the discussion,
> >
> > Git helps with this, but does not eliminate the problem. At some point
> the
> > changes become difficult to understand relative to the new code and may
> > even collide in ways that are difficult to merge.
> >
> > It is true, however, that patches can be kept live using tools like git.
> > That is how I kept the multi stuff alive, but there was at least one
> > tricky merge to be done.
> >
> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
> >
> >> Thanks to Ted for replying. I will save the mail I started in the drafts
> >> folder until I've calmed down again.
> >>
> >> Benjamin Reed:
> >> > deprioritizing them doesn't help because the patches themselves bit
> >> > rot. shortly they will not apply and then they will be worthless. the
> >> > poor contributor would then be left with the task of maintaining a
> >> > patch that would never commit.
> >> You should really give GIT a try. I've kept a pipeline of half a douzend
> >> patches filled and current over the last two months while drinking my
> >> morning
> >> coffee.
> >> Likewise have I updated my development branch of over a douzend commits
> >> every
> >> morning against the new ZK trunk.
> >>
> >> Regards,
> >>
> >> Thomas Koch, http://www.koch.ro
> >>
> >
>
-
Re: cleanup and subjective patchesPatrick Hunt 2011-11-01, 15:40
On Tue, Nov 1, 2011 at 7:21 AM, Fournier, Camille F.
<[EMAIL PROTECTED]> wrote:
> Personally, I have no problems with refactorings, but we seem to be spending much of our time dealing with them right now when we really need to get 3.4 out the door. It's been months that this release has been going on and splitting the committer attention between 3.4 and refactoring changes feels counterproductive to the needs of the community at this moment.
I second that. There are a few 3.4 blocker issues pending that need
review -- not to mention Mahadev hasn't gotten much feedback on the
release candidate.
There are 35 patch available issues at this moment, most of which are
_not_ refactoring issues. Plenty of patches for committers to review
if they are uninterested in aesthetic issues.
Patrick
>
> -----Original Message-----
> From: Ted Dunning [mailto:[EMAIL PROTECTED]]
> Sent: Monday, October 31, 2011 7:08 PM
> To: [EMAIL PROTECTED]
> Cc: [EMAIL PROTECTED]; Benjamin Reed
> Subject: Re: cleanup and subjective patches
>
> How about a middle ground being that aesthetic changes will only be
> considered if they bring significant new testing that helps mitigate the
> stability risk?
>
> On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
>
>> EOD this is not a tool problem, it's a resource issue. We have limited
>> committer time available for reviews. IMO Thomas's changes have been
>> pretty mechanical so far, they are easy to review/commit. They do
>> result in some patch churn but I've been surprised it's been so little
>> so far and the code is improving as a result. However we've just
>> scratched the surface (the easy stuff), I do have some concerns as we
>> make more significant structural changes. Personally I think we need
>> to focus on beefing up testing prior to making more significant
>> changes.
>>
>> Ben - We won't get new committers if we limit contributions. We'll
>> just dig a deeper hole for ourselves. Granted if we only review/accept
>> patches of a particular type (or from a particular person) we'll be in
>> a similar situation.
>>
>> Thomas - Ben has highlighted some good points, if we don't discuss
>> these rationally, out in the open, we won't be able to make progress.
>> There are other patches from other contributors that we need to
>> balance the available resources against.
>>
>> http://communityovercode.com/over/
>>
>>
>> Personally: my biggest concern is that we keep releasing solid high
>> quality code that works in production. I'm seeing patches go in that
>> break existing functionality and no one notices. To me the thing we
>> should be focusing on is adding testing. Refactoring and such to
>> improve/increase testing imo is a net positive.
>>
>> Patrick
>>
>> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]>
>> wrote:
>> > Jumping over to Ben's side of the discussion,
>> >
>> > Git helps with this, but does not eliminate the problem. At some point
>> the
>> > changes become difficult to understand relative to the new code and may
>> > even collide in ways that are difficult to merge.
>> >
>> > It is true, however, that patches can be kept live using tools like git.
>> > That is how I kept the multi stuff alive, but there was at least one
>> > tricky merge to be done.
>> >
>> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
>> >
>> >> Thanks to Ted for replying. I will save the mail I started in the drafts
>> >> folder until I've calmed down again.
>> >>
>> >> Benjamin Reed:
>> >> > deprioritizing them doesn't help because the patches themselves bit
>> >> > rot. shortly they will not apply and then they will be worthless. the
>> >> > poor contributor would then be left with the task of maintaining a
>> >> > patch that would never commit.
>> >> You should really give GIT a try. I've kept a pipeline of half a douzend
>> >> patches filled and current over the last two months while drinking my
>> >> morning
>> >> coffee.
>> >> Likewise have I updated my development branch of over a douzend commits
<[EMAIL PROTECTED]> wrote:
> Personally, I have no problems with refactorings, but we seem to be spending much of our time dealing with them right now when we really need to get 3.4 out the door. It's been months that this release has been going on and splitting the committer attention between 3.4 and refactoring changes feels counterproductive to the needs of the community at this moment.
I second that. There are a few 3.4 blocker issues pending that need
review -- not to mention Mahadev hasn't gotten much feedback on the
release candidate.
There are 35 patch available issues at this moment, most of which are
_not_ refactoring issues. Plenty of patches for committers to review
if they are uninterested in aesthetic issues.
Patrick
>
> -----Original Message-----
> From: Ted Dunning [mailto:[EMAIL PROTECTED]]
> Sent: Monday, October 31, 2011 7:08 PM
> To: [EMAIL PROTECTED]
> Cc: [EMAIL PROTECTED]; Benjamin Reed
> Subject: Re: cleanup and subjective patches
>
> How about a middle ground being that aesthetic changes will only be
> considered if they bring significant new testing that helps mitigate the
> stability risk?
>
> On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
>
>> EOD this is not a tool problem, it's a resource issue. We have limited
>> committer time available for reviews. IMO Thomas's changes have been
>> pretty mechanical so far, they are easy to review/commit. They do
>> result in some patch churn but I've been surprised it's been so little
>> so far and the code is improving as a result. However we've just
>> scratched the surface (the easy stuff), I do have some concerns as we
>> make more significant structural changes. Personally I think we need
>> to focus on beefing up testing prior to making more significant
>> changes.
>>
>> Ben - We won't get new committers if we limit contributions. We'll
>> just dig a deeper hole for ourselves. Granted if we only review/accept
>> patches of a particular type (or from a particular person) we'll be in
>> a similar situation.
>>
>> Thomas - Ben has highlighted some good points, if we don't discuss
>> these rationally, out in the open, we won't be able to make progress.
>> There are other patches from other contributors that we need to
>> balance the available resources against.
>>
>> http://communityovercode.com/over/
>>
>>
>> Personally: my biggest concern is that we keep releasing solid high
>> quality code that works in production. I'm seeing patches go in that
>> break existing functionality and no one notices. To me the thing we
>> should be focusing on is adding testing. Refactoring and such to
>> improve/increase testing imo is a net positive.
>>
>> Patrick
>>
>> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]>
>> wrote:
>> > Jumping over to Ben's side of the discussion,
>> >
>> > Git helps with this, but does not eliminate the problem. At some point
>> the
>> > changes become difficult to understand relative to the new code and may
>> > even collide in ways that are difficult to merge.
>> >
>> > It is true, however, that patches can be kept live using tools like git.
>> > That is how I kept the multi stuff alive, but there was at least one
>> > tricky merge to be done.
>> >
>> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
>> >
>> >> Thanks to Ted for replying. I will save the mail I started in the drafts
>> >> folder until I've calmed down again.
>> >>
>> >> Benjamin Reed:
>> >> > deprioritizing them doesn't help because the patches themselves bit
>> >> > rot. shortly they will not apply and then they will be worthless. the
>> >> > poor contributor would then be left with the task of maintaining a
>> >> > patch that would never commit.
>> >> You should really give GIT a try. I've kept a pipeline of half a douzend
>> >> patches filled and current over the last two months while drinking my
>> >> morning
>> >> coffee.
>> >> Likewise have I updated my development branch of over a douzend commits
-
Re: cleanup and subjective patchesThomas Koch 2011-11-01, 16:11
Fournier, Camille F.:
> Any changes coming in without tests should really be meaningfully
> untestable. I completely agree with the suggestion to require a testing
> uplift if you want to add refactorings unless you know the refactored code
> has 90+% test coverage.
>
> Personally, I have no problems with refactorings, but we seem to be
> spending much of our time dealing with them right now when we really need
> to get 3.4 out the door. It's been months that this release has been going
> on and splitting the committer attention between 3.4 and refactoring
> changes feels counterproductive to the needs of the community at this
> moment.
>
> C
Some facts for the last two months:
25 patches submitted by me were committed
added lines: 1805
deleted lines: 5795
==== -3990
Two patches were about big legacy code removals, excluding them:
added lines: 1770
deleted lines: 2013
==== -243
Critical blocker bugs found because of doing refactorings:
ZOOKEEPER-1269 Multi deserialization issues
ZOOKEEPER-1246 Dead code in PrepRequestProcessor catch Exception block
And two other possible issues described in mail
"Possible failure scenarios with deserialization + multi?"
minor bugs found:
ZOOKEEPER-1247 dead code in PrepRequestProcessor.pRequest multi case
ZOOKEEPER-1272 ZooKeeper.multi() could violate API if server misbehaves
ZOOKEEPER-1265 Normalize switch cases lists on request types
functional improvements:
ZOOKEEPER-1175 DataNode references parent node for no reason
ZOOKEEPER-556 Startup messages should account for common error of missing
leading slash in config files
missing tests added:
ZOOKEEPER-1254 test correct watch handling with multi ops
ZOOKEEPER-1259_central_mapping_from_type_to_txn_record_class (not yet
committed)
At the beginning of October I also reviewed _all_ open bugs and closed a
couple of obsolete onces or asked for feedback.
Regards,
Thomas Koch, http://www.koch.ro
> Any changes coming in without tests should really be meaningfully
> untestable. I completely agree with the suggestion to require a testing
> uplift if you want to add refactorings unless you know the refactored code
> has 90+% test coverage.
>
> Personally, I have no problems with refactorings, but we seem to be
> spending much of our time dealing with them right now when we really need
> to get 3.4 out the door. It's been months that this release has been going
> on and splitting the committer attention between 3.4 and refactoring
> changes feels counterproductive to the needs of the community at this
> moment.
>
> C
Some facts for the last two months:
25 patches submitted by me were committed
added lines: 1805
deleted lines: 5795
==== -3990
Two patches were about big legacy code removals, excluding them:
added lines: 1770
deleted lines: 2013
==== -243
Critical blocker bugs found because of doing refactorings:
ZOOKEEPER-1269 Multi deserialization issues
ZOOKEEPER-1246 Dead code in PrepRequestProcessor catch Exception block
And two other possible issues described in mail
"Possible failure scenarios with deserialization + multi?"
minor bugs found:
ZOOKEEPER-1247 dead code in PrepRequestProcessor.pRequest multi case
ZOOKEEPER-1272 ZooKeeper.multi() could violate API if server misbehaves
ZOOKEEPER-1265 Normalize switch cases lists on request types
functional improvements:
ZOOKEEPER-1175 DataNode references parent node for no reason
ZOOKEEPER-556 Startup messages should account for common error of missing
leading slash in config files
missing tests added:
ZOOKEEPER-1254 test correct watch handling with multi ops
ZOOKEEPER-1259_central_mapping_from_type_to_txn_record_class (not yet
committed)
At the beginning of October I also reviewed _all_ open bugs and closed a
couple of obsolete onces or asked for feedback.
Regards,
Thomas Koch, http://www.koch.ro
-
RE: cleanup and subjective patchesFournier, Camille F. 2011-11-01, 16:51
Thomas, we all agree that you have done useful work. But right now it seems that you are more concerned with the aesthetics of the code base than the correctness of it. You are capable of identifying potential bugs. Are you capable of doing the work to provably identify them (by writing tests) and fix them?
Now that you've drawn my attention to this, I have some questions. How the heck are ZOOKEEPER-1265 and ZOOKEEPER-1247 "bugfixes" if you fixed them without putting in a single test? That is not a bugfix. Nothing about those changes precluded writing tests for them, or modifying existing tests to prove the fix. It seems like perhaps they were viewed as refactoring, because otherwise I can't imagine why they weren't tested. You don't fix a bug unless you have a test that shows the bug by failing before the change and passing afterwards. Period. Please go back and write tests for these two fixes, otherwise I may revert the checkins and reopen the tickets.
Committers, this checking in of code without tests has got to stop. It's actively detrimental to the code base and undermines any value we might be getting from refactoring efforts. With very limited exceptions (changing socket parameters or something else that is virtually impossible to test without significant investment in additional libraries and scaffolding), we should not put in ANY more changes without tests. There's a reason we see a -1 in the Hudson build report for no new tests. Don't take it lightly.
C
-----Original Message-----
From: Thomas Koch [mailto:[EMAIL PROTECTED]]
Sent: Tuesday, November 01, 2011 12:12 PM
To: [EMAIL PROTECTED]
Cc: Fournier, Camille F. [Tech]; 'Benjamin Reed'
Subject: Re: cleanup and subjective patches
Fournier, Camille F.:
> Any changes coming in without tests should really be meaningfully
> untestable. I completely agree with the suggestion to require a testing
> uplift if you want to add refactorings unless you know the refactored code
> has 90+% test coverage.
>
> Personally, I have no problems with refactorings, but we seem to be
> spending much of our time dealing with them right now when we really need
> to get 3.4 out the door. It's been months that this release has been going
> on and splitting the committer attention between 3.4 and refactoring
> changes feels counterproductive to the needs of the community at this
> moment.
>
> C
Some facts for the last two months:
25 patches submitted by me were committed
added lines: 1805
deleted lines: 5795
==== -3990
Two patches were about big legacy code removals, excluding them:
added lines: 1770
deleted lines: 2013
==== -243
Critical blocker bugs found because of doing refactorings:
ZOOKEEPER-1269 Multi deserialization issues
ZOOKEEPER-1246 Dead code in PrepRequestProcessor catch Exception block
And two other possible issues described in mail
"Possible failure scenarios with deserialization + multi?"
minor bugs found:
ZOOKEEPER-1247 dead code in PrepRequestProcessor.pRequest multi case
ZOOKEEPER-1272 ZooKeeper.multi() could violate API if server misbehaves
ZOOKEEPER-1265 Normalize switch cases lists on request types
functional improvements:
ZOOKEEPER-1175 DataNode references parent node for no reason
ZOOKEEPER-556 Startup messages should account for common error of missing
leading slash in config files
missing tests added:
ZOOKEEPER-1254 test correct watch handling with multi ops
ZOOKEEPER-1259_central_mapping_from_type_to_txn_record_class (not yet
committed)
At the beginning of October I also reviewed _all_ open bugs and closed a
couple of obsolete onces or asked for feedback.
Regards,
Thomas Koch, http://www.koch.ro
Now that you've drawn my attention to this, I have some questions. How the heck are ZOOKEEPER-1265 and ZOOKEEPER-1247 "bugfixes" if you fixed them without putting in a single test? That is not a bugfix. Nothing about those changes precluded writing tests for them, or modifying existing tests to prove the fix. It seems like perhaps they were viewed as refactoring, because otherwise I can't imagine why they weren't tested. You don't fix a bug unless you have a test that shows the bug by failing before the change and passing afterwards. Period. Please go back and write tests for these two fixes, otherwise I may revert the checkins and reopen the tickets.
Committers, this checking in of code without tests has got to stop. It's actively detrimental to the code base and undermines any value we might be getting from refactoring efforts. With very limited exceptions (changing socket parameters or something else that is virtually impossible to test without significant investment in additional libraries and scaffolding), we should not put in ANY more changes without tests. There's a reason we see a -1 in the Hudson build report for no new tests. Don't take it lightly.
C
-----Original Message-----
From: Thomas Koch [mailto:[EMAIL PROTECTED]]
Sent: Tuesday, November 01, 2011 12:12 PM
To: [EMAIL PROTECTED]
Cc: Fournier, Camille F. [Tech]; 'Benjamin Reed'
Subject: Re: cleanup and subjective patches
Fournier, Camille F.:
> Any changes coming in without tests should really be meaningfully
> untestable. I completely agree with the suggestion to require a testing
> uplift if you want to add refactorings unless you know the refactored code
> has 90+% test coverage.
>
> Personally, I have no problems with refactorings, but we seem to be
> spending much of our time dealing with them right now when we really need
> to get 3.4 out the door. It's been months that this release has been going
> on and splitting the committer attention between 3.4 and refactoring
> changes feels counterproductive to the needs of the community at this
> moment.
>
> C
Some facts for the last two months:
25 patches submitted by me were committed
added lines: 1805
deleted lines: 5795
==== -3990
Two patches were about big legacy code removals, excluding them:
added lines: 1770
deleted lines: 2013
==== -243
Critical blocker bugs found because of doing refactorings:
ZOOKEEPER-1269 Multi deserialization issues
ZOOKEEPER-1246 Dead code in PrepRequestProcessor catch Exception block
And two other possible issues described in mail
"Possible failure scenarios with deserialization + multi?"
minor bugs found:
ZOOKEEPER-1247 dead code in PrepRequestProcessor.pRequest multi case
ZOOKEEPER-1272 ZooKeeper.multi() could violate API if server misbehaves
ZOOKEEPER-1265 Normalize switch cases lists on request types
functional improvements:
ZOOKEEPER-1175 DataNode references parent node for no reason
ZOOKEEPER-556 Startup messages should account for common error of missing
leading slash in config files
missing tests added:
ZOOKEEPER-1254 test correct watch handling with multi ops
ZOOKEEPER-1259_central_mapping_from_type_to_txn_record_class (not yet
committed)
At the beginning of October I also reviewed _all_ open bugs and closed a
couple of obsolete onces or asked for feedback.
Regards,
Thomas Koch, http://www.koch.ro
-
Re: cleanup and subjective patchesPatrick Hunt 2011-11-01, 17:15
On Tue, Nov 1, 2011 at 9:51 AM, Fournier, Camille F.
<[EMAIL PROTECTED]> wrote:
> Committers, this checking in of code without tests has got to stop. It's actively detrimental to the code base and undermines any value we might be getting from refactoring efforts. With very limited exceptions (changing socket parameters or something else that is virtually impossible to test without significant investment in additional libraries and scaffolding), we should not put in ANY more changes without tests. There's a reason we see a -1 in the Hudson build report for no new tests. Don't take it lightly.
There's a difference btw checking in refactorings and fixing warnings
and such that are covered by existing test, vs committing feature
changes and bug fixes that are not caught by existing tests.
Patrick
<[EMAIL PROTECTED]> wrote:
> Committers, this checking in of code without tests has got to stop. It's actively detrimental to the code base and undermines any value we might be getting from refactoring efforts. With very limited exceptions (changing socket parameters or something else that is virtually impossible to test without significant investment in additional libraries and scaffolding), we should not put in ANY more changes without tests. There's a reason we see a -1 in the Hudson build report for no new tests. Don't take it lightly.
There's a difference btw checking in refactorings and fixing warnings
and such that are covered by existing test, vs committing feature
changes and bug fixes that are not caught by existing tests.
Patrick
-
Re: cleanup and subjective patchesPatrick Hunt 2011-11-01, 17:32
On Tue, Nov 1, 2011 at 9:51 AM, Fournier, Camille F.
<[EMAIL PROTECTED]> wrote:
> Thomas, we all agree that you have done useful work. But right now it seems that you are more concerned with the aesthetics of the code base than the correctness of it. You are capable of identifying potential bugs. Are you capable of doing the work to provably identify them (by writing tests) and fix them?
Thomas has found some bugs - he's reported a few against multi support
for example. Perhaps he didn't feel enabled to work towards addressing
those.
Camille your point is a good one - the community is saying "we are
interested in addressing X", while Thomas has been addressing Y. If he
is interested to work on X I suspect he'll find broad support, if he
is only interested in Y I suspect it will continue to be an uphill
effort. My own opinion only, but I believe once X was addressed I'm
pretty sure people would be more open to doing this other work.
Q: Are we as a community clearly defining the work we are interested
in taking on in the near future?
Patrick
<[EMAIL PROTECTED]> wrote:
> Thomas, we all agree that you have done useful work. But right now it seems that you are more concerned with the aesthetics of the code base than the correctness of it. You are capable of identifying potential bugs. Are you capable of doing the work to provably identify them (by writing tests) and fix them?
Thomas has found some bugs - he's reported a few against multi support
for example. Perhaps he didn't feel enabled to work towards addressing
those.
Camille your point is a good one - the community is saying "we are
interested in addressing X", while Thomas has been addressing Y. If he
is interested to work on X I suspect he'll find broad support, if he
is only interested in Y I suspect it will continue to be an uphill
effort. My own opinion only, but I believe once X was addressed I'm
pretty sure people would be more open to doing this other work.
Q: Are we as a community clearly defining the work we are interested
in taking on in the near future?
Patrick
-
RE: cleanup and subjective patchesFournier, Camille F. 2011-11-01, 18:31
Sorry, you're right, let me rephrase: We shouldn't be checking in code that is claiming to fix bugs without tests. I also personally think that we should strongly encourage refactorings to come in with additional tests, as per Ted's suggestion. Leave the code base better than when you left it in a tangible way. There are two patches checked in that Thomas claims are bugfixes, that came in with no tests to verify that the bug they were fixing is actually fixed and had no test in place that was failing due to the bug.
C
-----Original Message-----
From: Patrick Hunt [mailto:[EMAIL PROTECTED]]
Sent: Tuesday, November 01, 2011 1:15 PM
To: [EMAIL PROTECTED]
Cc: [EMAIL PROTECTED]; Benjamin Reed
Subject: Re: cleanup and subjective patches
On Tue, Nov 1, 2011 at 9:51 AM, Fournier, Camille F.
<[EMAIL PROTECTED]> wrote:
> Committers, this checking in of code without tests has got to stop. It's actively detrimental to the code base and undermines any value we might be getting from refactoring efforts. With very limited exceptions (changing socket parameters or something else that is virtually impossible to test without significant investment in additional libraries and scaffolding), we should not put in ANY more changes without tests. There's a reason we see a -1 in the Hudson build report for no new tests. Don't take it lightly.
There's a difference btw checking in refactorings and fixing warnings
and such that are covered by existing test, vs committing feature
changes and bug fixes that are not caught by existing tests.
Patrick
C
-----Original Message-----
From: Patrick Hunt [mailto:[EMAIL PROTECTED]]
Sent: Tuesday, November 01, 2011 1:15 PM
To: [EMAIL PROTECTED]
Cc: [EMAIL PROTECTED]; Benjamin Reed
Subject: Re: cleanup and subjective patches
On Tue, Nov 1, 2011 at 9:51 AM, Fournier, Camille F.
<[EMAIL PROTECTED]> wrote:
> Committers, this checking in of code without tests has got to stop. It's actively detrimental to the code base and undermines any value we might be getting from refactoring efforts. With very limited exceptions (changing socket parameters or something else that is virtually impossible to test without significant investment in additional libraries and scaffolding), we should not put in ANY more changes without tests. There's a reason we see a -1 in the Hudson build report for no new tests. Don't take it lightly.
There's a difference btw checking in refactorings and fixing warnings
and such that are covered by existing test, vs committing feature
changes and bug fixes that are not caught by existing tests.
Patrick
-
RE: cleanup and subjective patchesFournier, Camille F. 2011-11-01, 18:39
Great way of phrasing it, Patrick. I agree. This community member thinks that the period around releases would be best spent concentrating on getting the release out the door, and the period immediately following releases is the best time to concentrate on new features, refactoring, and general code base cleanup.
And Thomas, if there's ever a bug you find but you don't feel enabled in whatever way to fix, feel free to ping me directly and I'm happy to help you get started.
C
-----Original Message-----
From: Patrick Hunt [mailto:[EMAIL PROTECTED]]
Sent: Tuesday, November 01, 2011 1:32 PM
To: [EMAIL PROTECTED]
Cc: [EMAIL PROTECTED]; Benjamin Reed
Subject: Re: cleanup and subjective patches
On Tue, Nov 1, 2011 at 9:51 AM, Fournier, Camille F.
<[EMAIL PROTECTED]> wrote:
> Thomas, we all agree that you have done useful work. But right now it seems that you are more concerned with the aesthetics of the code base than the correctness of it. You are capable of identifying potential bugs. Are you capable of doing the work to provably identify them (by writing tests) and fix them?
Thomas has found some bugs - he's reported a few against multi support
for example. Perhaps he didn't feel enabled to work towards addressing
those.
Camille your point is a good one - the community is saying "we are
interested in addressing X", while Thomas has been addressing Y. If he
is interested to work on X I suspect he'll find broad support, if he
is only interested in Y I suspect it will continue to be an uphill
effort. My own opinion only, but I believe once X was addressed I'm
pretty sure people would be more open to doing this other work.
Q: Are we as a community clearly defining the work we are interested
in taking on in the near future?
Patrick
And Thomas, if there's ever a bug you find but you don't feel enabled in whatever way to fix, feel free to ping me directly and I'm happy to help you get started.
C
-----Original Message-----
From: Patrick Hunt [mailto:[EMAIL PROTECTED]]
Sent: Tuesday, November 01, 2011 1:32 PM
To: [EMAIL PROTECTED]
Cc: [EMAIL PROTECTED]; Benjamin Reed
Subject: Re: cleanup and subjective patches
On Tue, Nov 1, 2011 at 9:51 AM, Fournier, Camille F.
<[EMAIL PROTECTED]> wrote:
> Thomas, we all agree that you have done useful work. But right now it seems that you are more concerned with the aesthetics of the code base than the correctness of it. You are capable of identifying potential bugs. Are you capable of doing the work to provably identify them (by writing tests) and fix them?
Thomas has found some bugs - he's reported a few against multi support
for example. Perhaps he didn't feel enabled to work towards addressing
those.
Camille your point is a good one - the community is saying "we are
interested in addressing X", while Thomas has been addressing Y. If he
is interested to work on X I suspect he'll find broad support, if he
is only interested in Y I suspect it will continue to be an uphill
effort. My own opinion only, but I believe once X was addressed I'm
pretty sure people would be more open to doing this other work.
Q: Are we as a community clearly defining the work we are interested
in taking on in the near future?
Patrick
-
Re: cleanup and subjective patchesPatrick Hunt 2011-11-01, 18:43
On Tue, Nov 1, 2011 at 11:31 AM, Fournier, Camille F.
<[EMAIL PROTECTED]> wrote:
> Sorry, you're right, let me rephrase: We shouldn't be checking in code that is claiming to fix bugs without tests. I also personally think that we should strongly encourage refactorings to come in with additional tests, as per Ted's suggestion. Leave the code base better than when you left it in a tangible way. There are two patches checked in that Thomas claims are bugfixes, that came in with no tests to verify that the bug they were fixing is actually fixed and had no test in place that was failing due to the bug.
I agree. I committed those - I'll be more diligent in future.
(although 1247 looks more like a cleanup than a specific fix)
Patrick
> -----Original Message-----
> From: Patrick Hunt [mailto:[EMAIL PROTECTED]]
> Sent: Tuesday, November 01, 2011 1:15 PM
> To: [EMAIL PROTECTED]
> Cc: [EMAIL PROTECTED]; Benjamin Reed
> Subject: Re: cleanup and subjective patches
>
> On Tue, Nov 1, 2011 at 9:51 AM, Fournier, Camille F.
> <[EMAIL PROTECTED]> wrote:
>> Committers, this checking in of code without tests has got to stop. It's actively detrimental to the code base and undermines any value we might be getting from refactoring efforts. With very limited exceptions (changing socket parameters or something else that is virtually impossible to test without significant investment in additional libraries and scaffolding), we should not put in ANY more changes without tests. There's a reason we see a -1 in the Hudson build report for no new tests. Don't take it lightly.
>
> There's a difference btw checking in refactorings and fixing warnings
> and such that are covered by existing test, vs committing feature
> changes and bug fixes that are not caught by existing tests.
>
> Patrick
>
<[EMAIL PROTECTED]> wrote:
> Sorry, you're right, let me rephrase: We shouldn't be checking in code that is claiming to fix bugs without tests. I also personally think that we should strongly encourage refactorings to come in with additional tests, as per Ted's suggestion. Leave the code base better than when you left it in a tangible way. There are two patches checked in that Thomas claims are bugfixes, that came in with no tests to verify that the bug they were fixing is actually fixed and had no test in place that was failing due to the bug.
I agree. I committed those - I'll be more diligent in future.
(although 1247 looks more like a cleanup than a specific fix)
Patrick
> -----Original Message-----
> From: Patrick Hunt [mailto:[EMAIL PROTECTED]]
> Sent: Tuesday, November 01, 2011 1:15 PM
> To: [EMAIL PROTECTED]
> Cc: [EMAIL PROTECTED]; Benjamin Reed
> Subject: Re: cleanup and subjective patches
>
> On Tue, Nov 1, 2011 at 9:51 AM, Fournier, Camille F.
> <[EMAIL PROTECTED]> wrote:
>> Committers, this checking in of code without tests has got to stop. It's actively detrimental to the code base and undermines any value we might be getting from refactoring efforts. With very limited exceptions (changing socket parameters or something else that is virtually impossible to test without significant investment in additional libraries and scaffolding), we should not put in ANY more changes without tests. There's a reason we see a -1 in the Hudson build report for no new tests. Don't take it lightly.
>
> There's a difference btw checking in refactorings and fixing warnings
> and such that are covered by existing test, vs committing feature
> changes and bug fixes that are not caught by existing tests.
>
> Patrick
>
