On 06/11/2013 07:00 PM, Junio C Hamano wrote: > Michael Haggerty <mhag...@alum.mit.edu> writes: > [...] >> * When reviewing other peoples' code, be tactful and constructive. Set >> high expectations, but do what you can to help the submitter achieve >> them. Don't demand changes based only on your personal preferences. >> Don't let the perfect be the enemy of the good. > > I think this is 30% aimed at me (as I think I do about that much of > the reviews around here). I fully agree with most of them, but the > last sentence is a bit too fuzzy to be a practically useful > guideline. Somebody's bare minimum is somebody else's perfection. > An unqualified "perfect is the enemy of good" is often incorrectly > used to justify "It works for me." and "There already are other > codepaths that do it in the same wrong way.", both of which make > things _worse_ for the long term project health.
I agree that the last line is fuzzy. And I don't think that I've observed any cases where I thought that reviewers were being too strict, so in a way it's just trying to head off hypothetical future problems and to make sure that the balance between submitter and reviewer is not *entirely* one-sided. Given our (proper, I think) strong deference to reviewers, one could imagine a reviewer abusing his/her authority to obstruct reasonable changes by (for example) making demands that the submitter also fix tangentially-related things that are beyond the scope of the patch. In my own projects I have a rough policy of "not worse than before", meaning that as long as a patch makes progress in at least one dimension, and doesn't make things worse in any other dimension, then it is acceptable. (Of course "worse" can include internal quality issues like copy-pasting code or even an increase in the amount of code disproportionate to its benefit.) A failure to make improvements in one area should not be a reason to block an improvement in another area, as long as nothing is made worse. But I can't right now think of a succinct way to express what I have in mind. >> * It is not OK to use these guidelines as a stick with which to beat >> supposed violators. However, if you genuinely feel that another >> community member is routinely behaving in ways that are detrimental to >> the community, it might help to calmly express your concerns to that >> person, preferably in a private email, and naming concrete and specific >> incidents rather than broad generalizations. > > I would think it is perfectly OK to say "The way you are refusing to > listen to constructive comments is not how things work around here" > by pointing at a set of guidelines. I agree. > Why do you think is it not OK? The "beating" part? I think it would be counterproductive for people to start saying things like "that is a violation of rule 3, section 2" *in everyday discussions*. This shouldn't be taken as a list of black-and-white laws, with allegations of small "infractions" used to shut down discussions. And on the other hand, if somebody shows a long history of acting contrary to the guidelines, and persists despite repeated requests to stop, I don't want the discussion to turn into a lawyerly analysis of the guidelines with point-by-point rebuttals and counter-rebuttals of whether this or that guideline was violated. The guidelines should just describe the expected tone of the community in a way that the vast majority of participants can agree on, and any kind of actions to enforce the guidelines should only be taken when an overwhelming majority of the community I think the CommunityGuidelines should have three main uses: 1. An artifact documenting the community consensus about what kinds of behaviors are encouraged and what kinds are considered unacceptable. It should only be accepted, and it only has value, if there is a strong consensus in favor of it. 2. A resource to help new community members get up to speed on our practices and expectations. 3. As a point of reference in the direst meltdowns, such as IMO we are having right now. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
