[lm-sensors] No longer reviewing random hwmon patches

[lm-sensors] No longer reviewing random hwmon patches

[Jean Delvare]

Hi all,

Twice in the past 2 weeks, I have been criticized for being too
demanding in my hwmon patch reviews. I have been told that I was
wasting developers' time and taking the fun away from them. Well, I am
also no longer having fun reviewing patches when I am "rewarded" this
way. Not that reviewing patches is really fun to start with, you
usually don't receive much thank nor praise, but receiving criticism is
going one step too far.

It has been suggested that I should review patches differently and
accept more divergence from what I consider the right way. This isn't
going to happen, sorry. Given that I am not paid for the job, nobody
gets to tell me which patches I should review and how. But I also don't
want to cause contributors to leave the project.

As a consequence, I have decided to change my behavior with regards to
the patches which are posted to the lm-sensors list. I will no longer
review random patches. I will only review patches meeting any of the
following conditions:
* I asked for that patch.
* The patch is for a hwmon driver I am maintaining.
* The author explicitly asked me to review his/her work, and I feel
  like it.
* Mark asked me to review the patch.

Hopefully this will prevent further tensions from arising.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] No longer reviewing random hwmon patches

[Hans-Jürgen Koch]

Am Mittwoch 18 Juli 2007 20:14 schrieb Jean Delvare:
> Hi all,
> 
> Twice in the past 2 weeks, I have been criticized for being too
> demanding in my hwmon patch reviews. I have been told that I was
> wasting developers' time and taking the fun away from them. Well, I am
> also no longer having fun reviewing patches when I am "rewarded" this
> way. Not that reviewing patches is really fun to start with, you
> usually don't receive much thank nor praise, but receiving criticism is
> going one step too far.

I absolutely agree. And I don't think you're wasting anybodies time.
I remember when you sent me back my max6650 patch 16 times. I still
think that this was no waste of time. It is now clean code which follows
a clean concept. A waste of time would have been to merge my first
version. It would have wasted the time of an unknown number of unknown
programmers in the future, who would have to deal with bad code
and violations of the hwmon sysfs specifications.

So, as I said before, I thank you for _your_ time and that you
made it possible for me to contribute a piece of _good_ code so
_my_ time was not wasted.  

> 
> It has been suggested that I should review patches differently and
> accept more divergence from what I consider the right way. This isn't
> going to happen, sorry. Given that I am not paid for the job, nobody
> gets to tell me which patches I should review and how. But I also don't
> want to cause contributors to leave the project.

If a "contributor" wants to leave because his crappy code is rejected,
I say: Good riddance! hwmon is a subsystem where most of the people
don't have the hardware to test a driver, so bugs can stay there for
a long time. That's why I find it especially important to have strict
reviews before something gets merged.

> 
> As a consequence, I have decided to change my behavior with regards to
> the patches which are posted to the lm-sensors list. I will no longer
> review random patches. 

I understand your point of view.

> I will only review patches meeting any of the 
> following conditions:
> * I asked for that patch.
> * The patch is for a hwmon driver I am maintaining.
> * The author explicitly asked me to review his/her work, and I feel
>   like it.
> * Mark asked me to review the patch.
> 
> Hopefully this will prevent further tensions from arising.
>

Actually, I think this is a big loss for hwmon. I'd still be glad if
you reviewed my patches (there will be more soon).

Thanks,
Hans



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] No longer reviewing random hwmon patches

[Roger Lucas]

> -----Original Message-----
> From: lm-sensors-bounces@lm-sensors.org [mailto:lm-sensors-bounces@lm-sensors.org] On Behalf Of
> Hans-Jürgen Koch
> Sent: 18 July 2007 20:27
> To: lm-sensors@lm-sensors.org
> Cc: Mark M. Hoffman
> Subject: Re: [lm-sensors] No longer reviewing random hwmon patches
> 
> Am Mittwoch 18 Juli 2007 20:14 schrieb Jean Delvare:
> > Hi all,
> >
> > Twice in the past 2 weeks, I have been criticized for being too
> > demanding in my hwmon patch reviews. I have been told that I was
> > wasting developers' time and taking the fun away from them. Well, I am
> > also no longer having fun reviewing patches when I am "rewarded" this
> > way. Not that reviewing patches is really fun to start with, you
> > usually don't receive much thank nor praise, but receiving criticism is
> > going one step too far.
> 
> I absolutely agree. And I don't think you're wasting anybodies time.
> I remember when you sent me back my max6650 patch 16 times. I still
> think that this was no waste of time. It is now clean code which follows
> a clean concept. A waste of time would have been to merge my first
> version. It would have wasted the time of an unknown number of unknown
> programmers in the future, who would have to deal with bad code
> and violations of the hwmon sysfs specifications.
> 
> So, as I said before, I thank you for _your_ time and that you
> made it possible for me to contribute a piece of _good_ code so
> _my_ time was not wasted.
> 

I absolutely agree.  If I am to run code on my systems, I want to be sure that it has been thoroughly reviewed by
whoever was responsible.  When I submitted my vt8231 driver, Jean was very thorough with his review, and I *appreciated*
this.

One of the problems with Linux's wider acceptance is that more and more people want to get involved.  This isn't a bad
thing in itself, but just because someone wants to help doesn't mean that their contribution must automatically be
accepted.  The reason Linux is growing in popularity is because it is so reliable.  People who want to have their code
should recognise this and accept that the standards for acceptace of code are high and are set by the reviewer, not the
submitter.

If you cannot take criticism of your code when you want to get it submitted into a larger system then it is you that has
the issues, not the reviewer or the review process.  I don't want to run code on my systems that has been through some
half-assed or incomplete review process.  I want to be confident that the code is written well and reviewed properly.

With Jean's attitude and dilligence, the review process was done properly.  He never criticised code for the sake of it
- every criticism was valid and relevant.  

> >
> > It has been suggested that I should review patches differently and
> > accept more divergence from what I consider the right way. This isn't
> > going to happen, sorry. Given that I am not paid for the job, nobody
> > gets to tell me which patches I should review and how. But I also don't
> > want to cause contributors to leave the project.
> 
> If a "contributor" wants to leave because his crappy code is rejected,
> I say: Good riddance! hwmon is a subsystem where most of the people
> don't have the hardware to test a driver, so bugs can stay there for
> a long time. That's why I find it especially important to have strict
> reviews before something gets merged.

Agreed.  If you are a prima donna with your code and either too arrogant or stupid to accept valid criticism then I for
one don't want to run it  - I don't trust code written by someone with that attitude!

> 
> >
> > As a consequence, I have decided to change my behavior with regards to
> > the patches which are posted to the lm-sensors list. I will no longer
> > review random patches.
> 
> I understand your point of view.
> 
> > I will only review patches meeting any of the
> > following conditions:
> > * I asked for that patch.
> > * The patch is for a hwmon driver I am maintaining.
> > * The author explicitly asked me to review his/her work, and I feel
> >   like it.
> > * Mark asked me to review the patch.
> >
> > Hopefully this will prevent further tensions from arising.
> >
> 
> Actually, I think this is a big loss for hwmon. I'd still be glad if
> you reviewed my patches (there will be more soon).
> 
> Thanks,
> Hans
> 

Jean, you have my support and my respect for your hard work and skills.  If others around you don’t realise what you
bring then it just shows how much they still have to learn.

I just hope that lm-sensors' code quality doesn't slowly degrade over time if the review process is relaxed.  Please
Mark H., follow Jean's prior practice at enforcing thorough reviews before accepting code into the kernel.

- Roger Lucas


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] No longer reviewing random hwmon patches

[Jean Delvare]

Hi Hans,

On Wed, 18 Jul 2007 21:27:17 +0200, Hans-Jürgen Koch wrote:
> I remember when you sent me back my max6650 patch 16 times.

You've got to be kidding ;)

> > I will only review patches meeting any of the 
> > following conditions:
> > * I asked for that patch.
> > * The patch is for a hwmon driver I am maintaining.
> > * The author explicitly asked me to review his/her work, and I feel
> >   like it.
> > * Mark asked me to review the patch.
> > 
> > Hopefully this will prevent further tensions from arising.
> 
> Actually, I think this is a big loss for hwmon. I'd still be glad if
> you reviewed my patches (there will be more soon).

See item #3 in my list: you only have to ask, and I'll review your
patch(es) as my time permits.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] No longer reviewing random hwmon patches

[David Hubbard]

Hi all,

Wow, I liked reading this thread. I just wanted to second (fifth?)
what has been said. Jean, your reviews of my patches are always
helpful and I have never felt unhappy with the stringent quality
standards. FreeBSD claims to be superior to Linux because only "well
written" code is accepted. I don't want to compare the two, but it's
great to be in a part of the Linux kernel where there is a high
standard of code quality. (On the other hand, since I haven't really
worked in many parts of the kernel, what do I know?)

So I will probably be requesting reviews from you in the future, when
I get time to write more code. :-)

David

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] No longer reviewing random hwmon patches

[JGong]

Hi Jean,

>Twice in the past 2 weeks, I have been criticized for being too
>demanding in my hwmon patch reviews. I have been told that I was
>wasting developers' time and taking the fun away from them. Well, I am
>also no longer having fun reviewing patches when I am "rewarded" this
>way. Not that reviewing patches is really fun to start with, you
>usually don't receive much thank nor praise, but receiving criticism is
>going one step too far.
>It has been suggested that I should review patches differently and
>accept more divergence from what I consider the right way. This isn't
>going to happen, sorry. Given that I am not paid for the job, nobody
>gets to tell me which patches I should review and how. But I also don't
>want to cause contributors to leave the project.
>As a consequence, I have decided to change my behavior with regards to
>the patches which are posted to the lm-sensors list. I will no longer
>review random patches. I will only review patches meeting any of the
>following conditions:
>* I asked for that patch.
>* The patch is for a hwmon driver I am maintaining.
>* The author explicitly asked me to review his/her work, and I feel
>  like it.
>* Mark asked me to review the patch.
>Hopefully this will prevent further tensions from arising.

You are always generous to help everybody in the list with any
questions. I must thank you for your steady support of our chips.

In my opinion, I think the contributor should understand that the code
quality is quite important and necessary especially when the code is
moving into the kernel tree. 
Fortunately, I have made a few good patches with your nice advice. :)

So I will continue to try my best to help you with the w83792d, w83793
chip's patch works if you need.

Best regards,
Gong Jun


=============================================The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such  a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Winbond is strictly prohibited; and any information in this email irrelevant to the official business of Winbond shall be deemed as neither given nor endorsed by Winbond.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] No longer reviewing random hwmon patches

[Till Harbaum / Lists]

Hi,

Am Mittwoch 18 Juli 2007 schrieb Hans-Jürgen Koch:
> I absolutely agree.
I just wanted to say: Me too!

I really appreciated your feedback and it was nice to get some
tipps from someone who knows this stuff better than me. That
indeed gave me the feeling that i wasn't submitting code
that included unnecessary glitches.

I am sure there are far more people appreciating your work than 
criticising it. The people complaining are usually louder
than the ones feeling comfortable with your work, so please
don't listen too much to these guys.

Till


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] No longer reviewing random hwmon patches

[Mark M. Hoffman]

Hi all:

* Roger Lucas <roger@planbit.co.uk> [2007-07-18 20:57:31 +0100]:
> I just hope that lm-sensors' code quality doesn't slowly degrade over time if
> the review process is relaxed.  Please Mark H., follow Jean's prior practice
> at enforcing thorough reviews before accepting code into the kernel.

I intend to do that, but I will need help.  At this time, there is no way I
could review all patches to the level of detail that Jean did and still does.

Also btw: I may be a bit less picky about some things than Jean... but I don't
want to hear whining about his or anyone else's reviews.  As a rule, each
review comment should be addressed as if the reviewer were ultimately the one
making the merge decision.  If an author flatly rejects a review, it should be
expected that the patch will drop to the end of my queue, if it's not deleted
outright - no matter who reviewed it.

There is a threshold beyond which I would theoretically step in and ask a
reviewer to back off - but it hasn't nearly been reached by Jean or anyone
else.

So, thank you to everyone who has stepped up with some reviews since I took
over.  When I'm more comfortable with the process and we've killed the patch
backlog, I will be doing more (but probably not all) reviews myself.  In the
meantime, I'm leaning on the rest of you.

Thanks & regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors