Open Stack

Excerpts from Sean Dague's message of 2014-06-21 05:08:01 -0700:
> On 06/20/2014 09:26 PM, Clint Byrum wrote:
> > Excerpts from Sean Dague's message of 2014-06-20 11:07:39 -0700:
> >>
> >> H803 - First line of a commit message must *not* end in a period. This
> >> was mostly a response to an unreasonable core reviewer that was -1ing
> >> people for not having periods. I think any core reviewer that -1s for
> >> this either way should be thrown off the island, or at least made fun
> >> of, a lot. Again, the clarity of a commit message is not made or lost by
> >> the lack or existence of a period at the end of the first line.
> >>
> > 
> > Perhaps we can make a bot that writes disparaging remarks on any -1's
> > that mention "period" in the line after the short commit message. :)
> > 

[For the reader: read the comments below, and then come back to this]

Note that I'm not at all unaware of the irony I created by making the
statement above and then the statements below. I feel like I'm a Fox
News reporter being called out on the daily show actually. ;)

> >> H305 - Enforcement of libraries fitting correctly into stdlib, 3rdparty,
> >> our tree. This biggest issue here is it's built in a world where there
> >> was only 1 viable python version, 2.7. Python's stdlib is actually
> >> pretty dynamic and grows over time. As we embrace more python 3, and as
> >> distros start to make python3 be front and center, what does this even
> >> mean? The current enforcement can't pass on both python2 and python3 at
> >> the same time in many cases because of that.
> >>
> > 
> > I think we should find a way to make this work. Like it or not, this
> > will garner -1's by people for stylistic reasons and I'd rather it be
> > the bots than the humans do it.
> > 
> > The algorithm is something like this pseudo python:
> > 
> > for block in import_blocks:
> >     if is_this_set_in_a_known_lib_collection(block):
> >         continue
> >     if is_this_set_entirely_local(block):
> >         continue
> >     if is_this_set_entirely_installed_libs(block):
> >         continue
> >     raise AnError(block)
> > 
> > And just make the python2 and python3 stdlibs both be a match. Basically
> > I'm saying, let's just be more forgiving but keep the check so we can
> > avoid most of the "-1 please group libs and stdlibs separately" patches.
> 
> You can avoid that by yelling at reviewers if that's the *only* feedback
> they are giving.
> 

I totally agree we can do that.

> Pedantic reviewers that are reviewing for this kind of thing only should
> be scorned. I realistically like the idea markmc came up with -
> https://twitter.com/markmc_/status/480073387600269312
>

I also agree it is really fun to think about shaming those annoying
actions. It is also not fun _at all_ to be publicly shamed. In fact I'd
say it is at least an order of magnitude less fun. There is an old saying,
"praise in public, punish in private." It is one reason the -1 comments I
give always include praise for whatever is right for new contributors. Not
everyone is a grizzled veteran.

It is far more interesting to me to solve the grouping problem in a
way that works for us long term (python 2 and 3) than it is to develop
a culture that builds any of its core activities on negative emotional
feedback.

That's not to say we can't say "hey you're doing it wrong." I mean to say
that direct feedback like that belongs in private IRC messages or email,
not in public "everyone can see that" reviews. Give people a chance to
save face. Meanwhile, the less we have to have one on one negative
feedback, the easier the job of reviewers is.

The last thing we want to do is have more reasons for people to NOT do
reviews.

> I no longer buy the theory that something like this is saving time. What
> it's actually doing is training a new generation of reviewers that the
> right thing to do it review for nits. That's not actually what we want,
> we want people reviewing for how this could go wrong.
> 

I'm not sure how hacking is training reviewers. I feel like hacking is
training developers. Reviewers don't even need to look at it until the
pep8 tox job passes.

> It's really instructive to realize that we've definitely gone beyond
> shared culture with what's in hacking. Look at how much of it is turned
> off in projects. It's pretty high. If this project is going to remain
> useful at all it really needs to prune back to what's actually shared
> culture.
>

I think having things turned off at the project level is o-k. The more
strict a project's automated style rules, the less they have to quibble
and train new reviewers on the fact that "we don't do that here."

However, I don't think rules being turned off is evidence that rules
are unhelpful. It most likely means that those rules didn't exist when
the code base was created and they turned them off because of incubation
or a new set of rules arrived and they didn't have time to land the new
patches. That is a per-project choice and should remain so, but I don't
think that choice means that those rules wouldn't have a long term
positive effect of stopping style -1 noise from overwhelming the review
signal. We simply cannot measure the effect each rule has since they
stop things from passing on the developer machine.

Anyway, I agree with you that we should be doing what we can to foster
positive energy into OpenStack. I think we just have different ideas on
how that happens.