BB
Ben Bucksch
Wed, Jun 12, 2019 12:49 PM
Dear TB reviewers,
I repeatedly see the phrase "we should change XYZ, while we're here". I
guess that comes from a notion of pressuring volunteers to make changes
that you know need to be done sometime, but the volunteer wouldn't
normally do, so you demand them as long as they are motivated for
another reason. That idea should no longer be relevant, given that there
are paid employees who can follow work orders to clean something up as a
later step 2.
Please resist the tendency to demand other changes that you notice
"while we're here". hg commits should be atomic, addressing a single
issue at a time. Atomic commits mean:
a) the patch is much easier to read, because it changes only a single
aspect or concern
b) the problem to solve is smaller and therefore easier to tackle, less
moving parts
c) there is less risk of regressions, and they are easier to handle,
because if something goes wrong, it's easier to pinpoint why it fails.
See https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention
Dear TB reviewers,
I repeatedly see the phrase "we should change XYZ, while we're here". I
guess that comes from a notion of pressuring volunteers to make changes
that you know need to be done sometime, but the volunteer wouldn't
normally do, so you demand them as long as they are motivated for
another reason. That idea should no longer be relevant, given that there
are paid employees who can follow work orders to clean something up as a
later step 2.
Please resist the tendency to demand other changes that you notice
"while we're here". hg commits should be atomic, addressing a single
issue at a time. Atomic commits mean:
a) the patch is much easier to read, because it changes only a single
aspect or concern
b) the problem to solve is smaller and therefore easier to tackle, less
moving parts
c) there is less risk of regressions, and they are easier to handle,
because if something goes wrong, it's easier to pinpoint why it fails.
See https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention
PK
Philipp Kewisch
Wed, Jun 12, 2019 4:35 PM
Hey Ben,
I think it sounds quite negative to assume writing a such comment is to pressure volunteers. There are certainly cases where fixing things while you are there makes sense, such as moving legacy code from one location to the next.
I agree making widespread changes in the whole file makes the commits more difficult to read though.
About the argument that we have paid people to do cleanups, I don't support making this distinction. The first rule of clean up later is it will never happen. I also don't think we should create any barriers between staff and volunteers, all in all they are all part of the community and nobody should feel they need to do the dirty work because others won't.
This is a good reminder to reviewers to consider whether requesting changes is reasonable though, thanks for posting.
Philipp
On 12. Jun 2019, at 2:49 PM, Ben Bucksch ben.bucksch@beonex.com wrote:
Dear TB reviewers,
I repeatedly see the phrase "we should change XYZ, while we're here". I guess that comes from a notion of pressuring volunteers to make changes that you know need to be done sometime, but the volunteer wouldn't normally do, so you demand them as long as they are motivated for another reason. That idea should no longer be relevant, given that there are paid employees who can follow work orders to clean something up as a later step 2.
Please resist the tendency to demand other changes that you notice "while we're here". hg commits should be atomic, addressing a single issue at a time. Atomic commits mean:
a) the patch is much easier to read, because it changes only a single aspect or concern
b) the problem to solve is smaller and therefore easier to tackle, less moving parts
c) there is less risk of regressions, and they are easier to handle, because if something goes wrong, it's easier to pinpoint why it fails.
See https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention
Maildev mailing list
Maildev@lists.thunderbird.net
http://lists.thunderbird.net/mailman/listinfo/maildev_lists.thunderbird.net
Hey Ben,
I think it sounds quite negative to assume writing a such comment is to pressure volunteers. There are certainly cases where fixing things while you are there makes sense, such as moving legacy code from one location to the next.
I agree making widespread changes in the whole file makes the commits more difficult to read though.
About the argument that we have paid people to do cleanups, I don't support making this distinction. The first rule of clean up later is it will never happen. I also don't think we should create any barriers between staff and volunteers, all in all they are all part of the community and nobody should feel they need to do the dirty work because others won't.
This is a good reminder to reviewers to consider whether requesting changes is reasonable though, thanks for posting.
Philipp
> On 12. Jun 2019, at 2:49 PM, Ben Bucksch <ben.bucksch@beonex.com> wrote:
>
> Dear TB reviewers,
>
> I repeatedly see the phrase "we should change XYZ, while we're here". I guess that comes from a notion of pressuring volunteers to make changes that you know need to be done sometime, but the volunteer wouldn't normally do, so you demand them as long as they are motivated for another reason. That idea should no longer be relevant, given that there are paid employees who can follow work orders to clean something up as a later step 2.
>
> Please resist the tendency to demand other changes that you notice "while we're here". hg commits should be atomic, addressing a single issue at a time. Atomic commits mean:
>
> a) the patch is much easier to read, because it changes only a single aspect or concern
>
> b) the problem to solve is smaller and therefore easier to tackle, less moving parts
>
> c) there is less risk of regressions, and they are easier to handle, because if something goes wrong, it's easier to pinpoint why it fails.
>
> See https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention
>
>
> _______________________________________________
> Maildev mailing list
> Maildev@lists.thunderbird.net
> http://lists.thunderbird.net/mailman/listinfo/maildev_lists.thunderbird.net
BB
Ben Bucksch
Wed, Jun 12, 2019 6:12 PM
Hello Philipp,
I think my choice of word "cleanup" was unfortunate. I did not refer to
making a mess with one commit and then leaving it to later to clean it
up. That's generally not a good idea (unless we have hard deadlines).
Staff or volunteer is not relevant in this case, either, I concur.
However, I was not talking about that.
I meant cases where old code does not conform to what the current
reviewer considers "good" code - for whatever reason. The contributor /
patch at hand has not created the issue, but it's pre-existing and not
relevant to the bug/feature to be fixed.
In these cases, I have very often seen reviewers asking or flat out
demanding the contributor to fix these unrelated issues in the same
patch. I think that's not fair to the contributor. But more importantly,
I'm pointing out that this is also bad from a pure engineering
perspective. It makes patches larger, with makes reviews longer, patches
more risky, and regression-hunting harder (you found the commit, but you
still don't know which change it was).
I am therefore proposing that Thunderbird reviews no longer do that.
Patches should be fixing a single issue only. Other unrelated and
pre-existing issues in the old code that are "wrong" (in the eyes of the
reviewer) should not be fixed in the same patch, but should always be
left to a followup bug with a different commit.
Here's a technical definition of "atomic commit" that I found is: 'a
single irreducible change'. A commit is atomic, if the patch cannot be
reduced further while still properly addressing the bug/feature at hand.
So, a clean and proper implementation of the feature can be included.
Other unrelated changes should be left out.
Philipp Kewisch wrote on 12.06.19 18:35:
Hey Ben,
I think it sounds quite negative to assume writing a such comment is to pressure volunteers. There are certainly cases where fixing things while you are there makes sense, such as moving legacy code from one location to the next.
I agree making widespread changes in the whole file makes the commits more difficult to read though.
About the argument that we have paid people to do cleanups, I don't support making this distinction. The first rule of clean up later is it will never happen. I also don't think we should create any barriers between staff and volunteers, all in all they are all part of the community and nobody should feel they need to do the dirty work because others won't.
This is a good reminder to reviewers to consider whether requesting changes is reasonable though, thanks for posting.
Philipp
On 12. Jun 2019, at 2:49 PM, Ben Bucksch ben.bucksch@beonex.com wrote:
Dear TB reviewers,
I repeatedly see the phrase "we should change XYZ, while we're here". I guess that comes from a notion of pressuring volunteers to make changes that you know need to be done sometime, but the volunteer wouldn't normally do, so you demand them as long as they are motivated for another reason. That idea should no longer be relevant, given that there are paid employees who can follow work orders to clean something up as a later step 2.
Please resist the tendency to demand other changes that you notice "while we're here". hg commits should be atomic, addressing a single issue at a time. Atomic commits mean:
a) the patch is much easier to read, because it changes only a single aspect or concern
b) the problem to solve is smaller and therefore easier to tackle, less moving parts
c) there is less risk of regressions, and they are easier to handle, because if something goes wrong, it's easier to pinpoint why it fails.
See https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention
Maildev mailing list
Maildev@lists.thunderbird.net
http://lists.thunderbird.net/mailman/listinfo/maildev_lists.thunderbird.net
Hello Philipp,
I think my choice of word "cleanup" was unfortunate. I did not refer to
making a mess with one commit and then leaving it to later to clean it
up. That's generally not a good idea (unless we have hard deadlines).
Staff or volunteer is not relevant in this case, either, I concur.
However, I was not talking about that.
I meant cases where old code does not conform to what the current
reviewer considers "good" code - for whatever reason. The contributor /
patch at hand has not created the issue, but it's pre-existing and not
relevant to the bug/feature to be fixed.
In these cases, I have very often seen reviewers asking or flat out
demanding the contributor to fix these unrelated issues in the same
patch. I think that's not fair to the contributor. But more importantly,
I'm pointing out that this is also bad from a pure engineering
perspective. It makes patches larger, with makes reviews longer, patches
more risky, and regression-hunting harder (you found the commit, but you
still don't know which change it was).
I am therefore proposing that Thunderbird reviews no longer do that.
Patches should be fixing a single issue only. Other unrelated and
pre-existing issues in the old code that are "wrong" (in the eyes of the
reviewer) should not be fixed in the same patch, but should always be
left to a followup bug with a different commit.
Here's a technical definition of "atomic commit" that I found is: 'a
single irreducible change'. A commit is atomic, if the patch cannot be
reduced further while still properly addressing the bug/feature at hand.
So, a clean and proper implementation of the feature can be included.
Other unrelated changes should be left out.
Philipp Kewisch wrote on 12.06.19 18:35:
> Hey Ben,
>
> I think it sounds quite negative to assume writing a such comment is to pressure volunteers. There are certainly cases where fixing things while you are there makes sense, such as moving legacy code from one location to the next.
>
> I agree making widespread changes in the whole file makes the commits more difficult to read though.
>
> About the argument that we have paid people to do cleanups, I don't support making this distinction. The first rule of clean up later is it will never happen. I also don't think we should create any barriers between staff and volunteers, all in all they are all part of the community and nobody should feel they need to do the dirty work because others won't.
>
> This is a good reminder to reviewers to consider whether requesting changes is reasonable though, thanks for posting.
>
> Philipp
>
>> On 12. Jun 2019, at 2:49 PM, Ben Bucksch <ben.bucksch@beonex.com> wrote:
>>
>> Dear TB reviewers,
>>
>> I repeatedly see the phrase "we should change XYZ, while we're here". I guess that comes from a notion of pressuring volunteers to make changes that you know need to be done sometime, but the volunteer wouldn't normally do, so you demand them as long as they are motivated for another reason. That idea should no longer be relevant, given that there are paid employees who can follow work orders to clean something up as a later step 2.
>>
>> Please resist the tendency to demand other changes that you notice "while we're here". hg commits should be atomic, addressing a single issue at a time. Atomic commits mean:
>>
>> a) the patch is much easier to read, because it changes only a single aspect or concern
>>
>> b) the problem to solve is smaller and therefore easier to tackle, less moving parts
>>
>> c) there is less risk of regressions, and they are easier to handle, because if something goes wrong, it's easier to pinpoint why it fails.
>>
>> See https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention
>>
>>
>> _______________________________________________
>> Maildev mailing list
>> Maildev@lists.thunderbird.net
>> http://lists.thunderbird.net/mailman/listinfo/maildev_lists.thunderbird.net
>
MM
Magnus Melin
Fri, Jun 14, 2019 11:37 AM
I think it's hard to have an exact rule for how atomic changes should
be. It really depends on what kind of bug it is, and other
circumstances. I agree in general it's good to try to keep changes
fairly focused on the issue at hand, but I think for practical purposes
it's often most time efficient to fix smaller things in the vicinity
while you're there. It's simply not time well spent if you first create
one bug to improve something, test that, and then go back and do another
patch + testing, when you could have tested it all at once.
I guess your post was related to the request to change <textbox> to
html:input for the new account setup while we're doing it. I still
think that's reasonable, since <textbox> is going away fast (within weeks).
-Magnus
On 12-06-2019 21:12, Ben Bucksch wrote:
Hello Philipp,
I think my choice of word "cleanup" was unfortunate. I did not refer
to making a mess with one commit and then leaving it to later to clean
it up. That's generally not a good idea (unless we have hard
deadlines). Staff or volunteer is not relevant in this case, either, I
concur. However, I was not talking about that.
I meant cases where old code does not conform to what the current
reviewer considers "good" code - for whatever reason. The contributor
/ patch at hand has not created the issue, but it's pre-existing and
not relevant to the bug/feature to be fixed.
In these cases, I have very often seen reviewers asking or flat out
demanding the contributor to fix these unrelated issues in the same
patch. I think that's not fair to the contributor. But more
importantly, I'm pointing out that this is also bad from a pure
engineering perspective. It makes patches larger, with makes reviews
longer, patches more risky, and regression-hunting harder (you found
the commit, but you still don't know which change it was).
I am therefore proposing that Thunderbird reviews no longer do that.
Patches should be fixing a single issue only. Other unrelated and
pre-existing issues in the old code that are "wrong" (in the eyes of
the reviewer) should not be fixed in the same patch, but should always
be left to a followup bug with a different commit.
Here's a technical definition of "atomic commit" that I found is: 'a
single irreducible change'. A commit is atomic, if the patch cannot be
reduced further while still properly addressing the bug/feature at hand.
So, a clean and proper implementation of the feature can be included.
Other unrelated changes should be left out.
Philipp Kewisch wrote on 12.06.19 18:35:
Hey Ben,
I think it sounds quite negative to assume writing a such comment is
to pressure volunteers. There are certainly cases where fixing things
while you are there makes sense, such as moving legacy code from one
location to the next.
I agree making widespread changes in the whole file makes the commits
more difficult to read though.
About the argument that we have paid people to do cleanups, I don't
support making this distinction. The first rule of clean up later is
it will never happen. I also don't think we should create any
barriers between staff and volunteers, all in all they are all part
of the community and nobody should feel they need to do the dirty
work because others won't.
This is a good reminder to reviewers to consider whether requesting
changes is reasonable though, thanks for posting.
Philipp
On 12. Jun 2019, at 2:49 PM, Ben Bucksch ben.bucksch@beonex.com
wrote:
Dear TB reviewers,
I repeatedly see the phrase "we should change XYZ, while we're
here". I guess that comes from a notion of pressuring volunteers to
make changes that you know need to be done sometime, but the
volunteer wouldn't normally do, so you demand them as long as they
are motivated for another reason. That idea should no longer be
relevant, given that there are paid employees who can follow work
orders to clean something up as a later step 2.
Please resist the tendency to demand other changes that you notice
"while we're here". hg commits should be atomic, addressing a single
issue at a time. Atomic commits mean:
a) the patch is much easier to read, because it changes only a
single aspect or concern
b) the problem to solve is smaller and therefore easier to tackle,
less moving parts
c) there is less risk of regressions, and they are easier to handle,
because if something goes wrong, it's easier to pinpoint why it fails.
See
https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention
Maildev mailing list
Maildev@lists.thunderbird.net
http://lists.thunderbird.net/mailman/listinfo/maildev_lists.thunderbird.net
I think it's hard to have an exact rule for how atomic changes should
be. It really depends on what kind of bug it is, and other
circumstances. I agree in general it's good to try to keep changes
fairly focused on the issue at hand, but I think for practical purposes
it's often most time efficient to fix smaller things in the vicinity
while you're there. It's simply not time well spent if you first create
one bug to improve something, test that, and then go back and do another
patch + testing, when you could have tested it all at once.
I guess your post was related to the request to change <textbox> to
<html:input> for the new account setup while we're doing it. I still
think that's reasonable, since <textbox> is going away fast (within weeks).
-Magnus
On 12-06-2019 21:12, Ben Bucksch wrote:
> Hello Philipp,
>
> I think my choice of word "cleanup" was unfortunate. I did not refer
> to making a mess with one commit and then leaving it to later to clean
> it up. That's generally not a good idea (unless we have hard
> deadlines). Staff or volunteer is not relevant in this case, either, I
> concur. However, I was not talking about that.
>
> I meant cases where old code does not conform to what the current
> reviewer considers "good" code - for whatever reason. The contributor
> / patch at hand has not created the issue, but it's pre-existing and
> not relevant to the bug/feature to be fixed.
>
> In these cases, I have very often seen reviewers asking or flat out
> demanding the contributor to fix these unrelated issues in the same
> patch. I think that's not fair to the contributor. But more
> importantly, I'm pointing out that this is also bad from a pure
> engineering perspective. It makes patches larger, with makes reviews
> longer, patches more risky, and regression-hunting harder (you found
> the commit, but you still don't know which change it was).
>
> I am therefore proposing that Thunderbird reviews no longer do that.
> Patches should be fixing a single issue only. Other unrelated and
> pre-existing issues in the old code that are "wrong" (in the eyes of
> the reviewer) should not be fixed in the same patch, but should always
> be left to a followup bug with a different commit.
>
> Here's a technical definition of "atomic commit" that I found is: 'a
> single irreducible change'. A commit is atomic, if the patch cannot be
> reduced further while still properly addressing the bug/feature at hand.
>
> So, a clean and proper implementation of the feature can be included.
> Other unrelated changes should be left out.
>
>
> Philipp Kewisch wrote on 12.06.19 18:35:
>> Hey Ben,
>>
>> I think it sounds quite negative to assume writing a such comment is
>> to pressure volunteers. There are certainly cases where fixing things
>> while you are there makes sense, such as moving legacy code from one
>> location to the next.
>>
>> I agree making widespread changes in the whole file makes the commits
>> more difficult to read though.
>>
>> About the argument that we have paid people to do cleanups, I don't
>> support making this distinction. The first rule of clean up later is
>> it will never happen. I also don't think we should create any
>> barriers between staff and volunteers, all in all they are all part
>> of the community and nobody should feel they need to do the dirty
>> work because others won't.
>>
>> This is a good reminder to reviewers to consider whether requesting
>> changes is reasonable though, thanks for posting.
>>
>> Philipp
>>
>>> On 12. Jun 2019, at 2:49 PM, Ben Bucksch <ben.bucksch@beonex.com>
>>> wrote:
>>>
>>> Dear TB reviewers,
>>>
>>> I repeatedly see the phrase "we should change XYZ, while we're
>>> here". I guess that comes from a notion of pressuring volunteers to
>>> make changes that you know need to be done sometime, but the
>>> volunteer wouldn't normally do, so you demand them as long as they
>>> are motivated for another reason. That idea should no longer be
>>> relevant, given that there are paid employees who can follow work
>>> orders to clean something up as a later step 2.
>>>
>>> Please resist the tendency to demand other changes that you notice
>>> "while we're here". hg commits should be atomic, addressing a single
>>> issue at a time. Atomic commits mean:
>>>
>>> a) the patch is much easier to read, because it changes only a
>>> single aspect or concern
>>>
>>> b) the problem to solve is smaller and therefore easier to tackle,
>>> less moving parts
>>>
>>> c) there is less risk of regressions, and they are easier to handle,
>>> because if something goes wrong, it's easier to pinpoint why it fails.
>>>
>>> See
>>> https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention
>>>
>>>
>>> _______________________________________________
>>> Maildev mailing list
>>> Maildev@lists.thunderbird.net
>>> http://lists.thunderbird.net/mailman/listinfo/maildev_lists.thunderbird.net
>>>
>>
>
>
> _______________________________________________
> Maildev mailing list
> Maildev@lists.thunderbird.net
> http://lists.thunderbird.net/mailman/listinfo/maildev_lists.thunderbird.net
>
JK
Jörg Knobloch
Sat, Jun 15, 2019 9:37 AM
On 12 Jun 2019 14:49, Ben Bucksch wrote:
Please resist the tendency to demand other changes that you notice
"while we're here".
Hi Ben,
without seeing the particular case, this is hard to judge. Sadly, in
parts, we have a very old code base, so "while we are there", we need to
do a little clean-up. As you pointed out, mixing clean-up and real
change can cloud the issue, so in many bugs two or more patches are
posted, first the clean-up, then the real change. I could find various
examples for that.
"Demand" sounds quite nasty. If the patch is provided by a staff member,
there is no problem in asking for a related clean-up. If the patch is
provided by a volunteer, the reviewer should motivate why some clean-up
is needed before getting to the issue, or the reviewer could provide the
clean-up part himself/herself. Contributing voluntarily means having the
future of the product in mind, and that includes maintenance clean-up.
We also appreciate the so-called "boy scout" approach, that is, leaving
the site cleaner than you found it.
Apart from paid staff and volunteers there is a third group of
contributors which is mostly you and Neil. You work on TB since you have
and add-on to generate revenue for you which TB must support.
This mailing list is the official voice of the (never fully established)
Engineering Steering Committee, so let me say this: I am disappointed
that the Thunderbird Council did not enter a contract with you that
would have obligated you to the maintenance to JS Account. You and your
team show up immediately when your product is affected, see for
example[1], but there is room for improvement in other cases, see for
example[2][3][4][5].
So in summary: Atomic changes: Yes, "white we're here" clean-up changes
"demanded" of people who make money with the product: Absolutely!!
Jörg.
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1555633
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1447492 - JS Account
[3]
https://bugzilla.mozilla.org/request.cgi?action=queue&type=review&requestee=neil%40httl.net&group=type
[4]
https://bugzilla.mozilla.org/request.cgi?action=queue&type=review&requestee=ben.bucksch%40beonex.com&group=type
[5] https://mzl.la/2KkpIPv
On 12 Jun 2019 14:49, Ben Bucksch wrote:
> Please resist the tendency to demand other changes that you notice
> "while we're here".
Hi Ben,
without seeing the particular case, this is hard to judge. Sadly, in
parts, we have a very old code base, so "while we are there", we need to
do a little clean-up. As you pointed out, mixing clean-up and real
change can cloud the issue, so in many bugs two or more patches are
posted, first the clean-up, then the real change. I could find various
examples for that.
"Demand" sounds quite nasty. If the patch is provided by a staff member,
there is no problem in asking for a related clean-up. If the patch is
provided by a volunteer, the reviewer should motivate why some clean-up
is needed before getting to the issue, or the reviewer could provide the
clean-up part himself/herself. Contributing voluntarily means having the
future of the product in mind, and that includes maintenance clean-up.
We also appreciate the so-called "boy scout" approach, that is, leaving
the site cleaner than you found it.
Apart from paid staff and volunteers there is a third group of
contributors which is mostly you and Neil. You work on TB since you have
and add-on to generate revenue for you which TB must support.
This mailing list is the official voice of the (never fully established)
Engineering Steering Committee, so let me say this: I am disappointed
that the Thunderbird Council did not enter a contract with you that
would have obligated you to the maintenance to JS Account. You and your
team show up immediately when your product is affected, see for
example[1], but there is room for improvement in other cases, see for
example[2][3][4][5].
So in summary: Atomic changes: Yes, "white we're here" clean-up changes
"demanded" of people who make money with the product: Absolutely!!
Jörg.
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1555633
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1447492 - JS Account
[3]
https://bugzilla.mozilla.org/request.cgi?action=queue&type=review&requestee=neil%40httl.net&group=type
[4]
https://bugzilla.mozilla.org/request.cgi?action=queue&type=review&requestee=ben.bucksch%40beonex.com&group=type
[5] https://mzl.la/2KkpIPv
BB
Ben Bucksch
Wed, Jun 19, 2019 1:54 PM
Magnus Melin wrote on 14.06.19 13:37:
I think it's hard to have an exact rule for how atomic changes should
be. It really depends on what kind of bug it is, and other
circumstances. I agree in general it's good to try to keep changes
fairly focused on the issue at hand, but I think for practical
purposes it's often most time efficient to fix smaller things in the
vicinity while you're there. It's simply not time well spent if you
first create one bug to improve something, test that, and then go back
and do another patch + testing, when you could have tested it all at
once.
Agreed. When the developer sees a small and obvious issue while he's
working on something else, and the effect is confined, it saves
developer time to include it. I concur there.
However, this should be avoided by the developer, if a) the changes
touch a lot of code, i.e. make the diff harder to read or b) the change,
although small code footprint, have a high risk of causing other
regressions and the change is not necessary to attain the original
purpose of the bug.
The "saves developer time" is turned into the opposite, if the reviewer
demands the change. This is extremely time-consuming for the developer,
and frustrating, because a) it increases the number of review rounds,
which is something very expensive for the developer, esp. when we work
at different times and the developer has to wait hours or days for the
next review round b) it increases the scope, work and risk of the patch
in a direction that is not necessary for the original bug (which
violates he atomic commit idea) and c) the developer or another reviewer
might agree with that request, which adds completely unnecessary
discussion which has nothing to do with the original bug and in fact
de-rails the bug.
So, for a developer it's important to keep focus on the original purpose
of the change. But developers naturally do that. What's mostly dangerous
is when reviewers derail things. The developer is helpless towards the
reviewer, because he just wants his change landed, so I think that's
neither good for developer morale, nor for engineering practices - too
many unrelated changes in the same commit.
I guess your post was related to the request to change <textbox> to
html:input for the new account setup while we're doing it. I still
think that's reasonable, since <textbox> is going away fast (within
weeks).
Of course you think that's reasonable, because you proposed that. But
this is a good example of what to never do. The purpose of that change
was a pure styling change. The change to html:input may be important and
imminent, yes, but even so, it's much better to do it as separate
changesets. Imagine there's a regression and the change needs to be
backed out. Then everything gets backed out at once. Also, regression
testing is much harder: How do you know what the cause is, if you have
so many substantial changes mixed together?
The patch was done and working without the html:input change. It didn't
help to mix that substantial change into another big substantial code
change, but we should do one at a time. First we finish the style
changes (which we did), then we land that (which we did not), then we
make the html:input change, then we land that.
That was one particularly obvious case, but I see "reviewer forces
unrelated changes onto developer" as a common mis-pattern that has
established here (some reviewers did that, so the other reviewers do it
as well now), and it's highly damaging.
Ben Bucksch
Magnus Melin wrote on 14.06.19 13:37:
> I think it's hard to have an exact rule for how atomic changes should
> be. It really depends on what kind of bug it is, and other
> circumstances. I agree in general it's good to try to keep changes
> fairly focused on the issue at hand, but I think for practical
> purposes it's often most time efficient to fix smaller things in the
> vicinity while you're there. It's simply not time well spent if you
> first create one bug to improve something, test that, and then go back
> and do another patch + testing, when you could have tested it all at
> once.
Agreed. When the developer sees a small and obvious issue while he's
working on something else, and the effect is confined, it saves
developer time to include it. I concur there.
However, this should be avoided by the developer, if a) the changes
touch a lot of code, i.e. make the diff harder to read or b) the change,
although small code footprint, have a high risk of causing other
regressions and the change is not necessary to attain the original
purpose of the bug.
The "saves developer time" is turned into the opposite, if the reviewer
demands the change. This is extremely time-consuming for the developer,
and frustrating, because a) it increases the number of review rounds,
which is something very expensive for the developer, esp. when we work
at different times and the developer has to wait hours or days for the
next review round b) it increases the scope, work and risk of the patch
in a direction that is not necessary for the original bug (which
violates he atomic commit idea) and c) the developer or another reviewer
might agree with that request, which adds completely unnecessary
discussion which has nothing to do with the original bug and in fact
de-rails the bug.
So, for a developer it's important to keep focus on the original purpose
of the change. But developers naturally do that. What's mostly dangerous
is when reviewers derail things. The developer is helpless towards the
reviewer, because he just wants his change landed, so I think that's
neither good for developer morale, nor for engineering practices - too
many unrelated changes in the same commit.
> I guess your post was related to the request to change <textbox> to
> <html:input> for the new account setup while we're doing it. I still
> think that's reasonable, since <textbox> is going away fast (within
> weeks).
Of course you think that's reasonable, because you proposed that. But
this is a good example of what to never do. The purpose of that change
was a pure styling change. The change to html:input may be important and
imminent, yes, but even so, it's much better to do it as separate
changesets. Imagine there's a regression and the change needs to be
backed out. Then everything gets backed out at once. Also, regression
testing is much harder: How do you know what the cause is, if you have
so many substantial changes mixed together?
The patch was done and working without the html:input change. It didn't
help to mix that substantial change into another big substantial code
change, but we should do one at a time. First we finish the style
changes (which we did), then we land that (which we did not), then we
make the html:input change, then we land that.
That was one particularly obvious case, but I see "reviewer forces
unrelated changes onto developer" as a common mis-pattern that has
established here (some reviewers did that, so the other reviewers do it
as well now), and it's highly damaging.
Ben Bucksch
